* [PATCH 0/2] TTY: add rpmsg tty driver
@ 2019-03-21 15:47 Fabien Dessenne
2019-03-21 15:47 ` [PATCH 1/2] rpmsg: core: add possibility to get message payload length Fabien Dessenne
2019-03-21 15:47 ` [PATCH 2/2] tty: add rpmsg driver Fabien Dessenne
0 siblings, 2 replies; 19+ messages in thread
From: Fabien Dessenne @ 2019-03-21 15:47 UTC (permalink / raw)
To: Ohad Ben-Cohen, Bjorn Andersson, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel, linux-remoteproc
Cc: Fabien Dessenne, Benjamin Gaignard, Arnaud Pouliquen
This patch set introduces a TTY console on top of the RPMsg framework
which enables the following use cases:
- Provide a console to communicate easily with the remote processor
application.
- Provide an interface to get the remote processor log traces without
ring buffer limitation.
- Ease the migration from MPU + MCU processors to multi core processors
(MPU and MCU integrated in one processor)
An alternative of this proposed solution would consist in using the virtio
console : the drawback with that solution is that it requires a specific
virtio buffer (in addition to the one already used for RPMsg) which does
not fit with remote processors with little memory.
The proposed solution allows to multiplex the console with the other rpmsg
services, optimizing the memory.
The first patch adds an API to the rpmsg framework ('get buffer size') and
the second one is the rpmsg tty driver itself.
Fabien Dessenne (2):
rpmsg: core: add possibility to get message payload length
tty: add rpmsg driver
drivers/rpmsg/rpmsg_core.c | 20 +++
drivers/rpmsg/rpmsg_internal.h | 2 +
drivers/rpmsg/virtio_rpmsg_bus.c | 11 ++
drivers/tty/Kconfig | 9 ++
drivers/tty/Makefile | 1 +
drivers/tty/rpmsg_tty.c | 309 +++++++++++++++++++++++++++++++++++++++
include/linux/rpmsg.h | 10 ++
7 files changed, 362 insertions(+)
create mode 100644 drivers/tty/rpmsg_tty.c
--
2.7.4
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/2] rpmsg: core: add possibility to get message payload length
2019-03-21 15:47 [PATCH 0/2] TTY: add rpmsg tty driver Fabien Dessenne
@ 2019-03-21 15:47 ` Fabien Dessenne
2019-03-21 15:47 ` [PATCH 2/2] tty: add rpmsg driver Fabien Dessenne
1 sibling, 0 replies; 19+ messages in thread
From: Fabien Dessenne @ 2019-03-21 15:47 UTC (permalink / raw)
To: Ohad Ben-Cohen, Bjorn Andersson, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel, linux-remoteproc
Cc: Fabien Dessenne, Benjamin Gaignard, Arnaud Pouliquen
Return the rpmsg buffer payload size for sending message, so rpmsg users
can split a long message in several sub rpmsg buffers.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
---
drivers/rpmsg/rpmsg_core.c | 20 ++++++++++++++++++++
drivers/rpmsg/rpmsg_internal.h | 2 ++
drivers/rpmsg/virtio_rpmsg_bus.c | 11 +++++++++++
include/linux/rpmsg.h | 10 ++++++++++
4 files changed, 43 insertions(+)
diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 8122807..75c8c40 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -283,6 +283,26 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
}
EXPORT_SYMBOL(rpmsg_trysend_offchannel);
+/**
+ * rpmsg_get_buf_payload_size()
+ * This function returns buffer payload size available for sending messages.
+ *
+ * @ept: the rpmsg endpoint
+ *
+ * Returns buffer payload size on success and an appropriate error value on
+ * failure.
+ */
+int rpmsg_get_buf_payload_size(struct rpmsg_endpoint *ept)
+{
+ if (WARN_ON(!ept))
+ return -EINVAL;
+ if (!ept->ops->get_buf_payload_size)
+ return -ENXIO;
+
+ return ept->ops->get_buf_payload_size(ept);
+}
+EXPORT_SYMBOL(rpmsg_get_buf_payload_size);
+
/*
* match an rpmsg channel with a channel info struct.
* this is used to make sure we're not creating rpmsg devices for channels
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 0d791c3..6f733a5 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -46,6 +46,7 @@ struct rpmsg_device_ops {
* @trysend: see @rpmsg_trysend(), required
* @trysendto: see @rpmsg_trysendto(), optional
* @trysend_offchannel: see @rpmsg_trysend_offchannel(), optional
+ * @get_buf_payload_size: see @rpmsg_get_buf_payload_size(), optional
*
* Indirection table for the operations that a rpmsg backend should implement.
* In addition to @destroy_ept, the backend must at least implement @send and
@@ -65,6 +66,7 @@ struct rpmsg_endpoint_ops {
void *data, int len);
__poll_t (*poll)(struct rpmsg_endpoint *ept, struct file *filp,
poll_table *wait);
+ int (*get_buf_payload_size)(struct rpmsg_endpoint *ept);
};
int rpmsg_register_device(struct rpmsg_device *rpdev);
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 5d3685b..82753e7 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -175,6 +175,7 @@ static int virtio_rpmsg_trysendto(struct rpmsg_endpoint *ept, void *data,
int len, u32 dst);
static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
u32 dst, void *data, int len);
+static int virtio_get_buf_payload_size(struct rpmsg_endpoint *ept);
static const struct rpmsg_endpoint_ops virtio_endpoint_ops = {
.destroy_ept = virtio_rpmsg_destroy_ept,
@@ -184,6 +185,7 @@ static const struct rpmsg_endpoint_ops virtio_endpoint_ops = {
.trysend = virtio_rpmsg_trysend,
.trysendto = virtio_rpmsg_trysendto,
.trysend_offchannel = virtio_rpmsg_trysend_offchannel,
+ .get_buf_payload_size = virtio_get_buf_payload_size,
};
/**
@@ -699,6 +701,15 @@ static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
}
+static int virtio_get_buf_payload_size(struct rpmsg_endpoint *ept)
+{
+ struct rpmsg_device *rpdev = ept->rpdev;
+ struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
+ int size = vch->vrp->buf_size - sizeof(struct rpmsg_hdr);
+
+ return size < 0 ? -EMSGSIZE : size;
+}
+
static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
struct rpmsg_hdr *msg, unsigned int len)
{
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 9fe156d..250df21 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -135,6 +135,8 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
__poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
poll_table *wait);
+int rpmsg_get_buf_payload_size(struct rpmsg_endpoint *ept);
+
#else
static inline int register_rpmsg_device(struct rpmsg_device *dev)
@@ -242,6 +244,14 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
return 0;
}
+static int rpmsg_get_buf_payload_size(struct rpmsg_endpoint *ept)
+{
+ /* This shouldn't be possible */
+ WARN_ON(1);
+
+ return -ENXIO;
+}
+
#endif /* IS_ENABLED(CONFIG_RPMSG) */
/* use a macro to avoid include chaining to get THIS_MODULE */
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/2] tty: add rpmsg driver
2019-03-21 15:47 [PATCH 0/2] TTY: add rpmsg tty driver Fabien Dessenne
2019-03-21 15:47 ` [PATCH 1/2] rpmsg: core: add possibility to get message payload length Fabien Dessenne
@ 2019-03-21 15:47 ` Fabien Dessenne
2019-04-03 8:44 ` Johan Hovold
2019-04-03 12:47 ` xiang xiao
1 sibling, 2 replies; 19+ messages in thread
From: Fabien Dessenne @ 2019-03-21 15:47 UTC (permalink / raw)
To: Ohad Ben-Cohen, Bjorn Andersson, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel, linux-remoteproc
Cc: Fabien Dessenne, Benjamin Gaignard, Arnaud Pouliquen
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@st.com>
Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
---
drivers/tty/Kconfig | 9 ++
drivers/tty/Makefile | 1 +
drivers/tty/rpmsg_tty.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 319 insertions(+)
create mode 100644 drivers/tty/rpmsg_tty.c
diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index 0840d27..42696e6 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -441,4 +441,13 @@ config VCC
depends on SUN_LDOMS
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.
endif # TTY
diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
index c72cafd..90a98a2 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
new file mode 100644
index 0000000..9c2a629
--- /dev/null
+++ b/drivers/tty/rpmsg_tty.c
@@ -0,0 +1,309 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) STMicroelectronics 2019 - All Rights Reserved
+ * Authors: Arnaud Pouliquen <arnaud.pouliquen@st.com> for STMicroelectronics.
+ * Fabien Dessenne <fabien.dessenne@st.com> for STMicroelectronics.
+ * Derived from the imx-rpmsg and omap-rpmsg implementations.
+ */
+
+#include <linux/module.h>
+#include <linux/rpmsg.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+
+#define MAX_TTY_RPMSG 32
+
+static DEFINE_IDR(tty_idr); /* tty instance id */
+static DEFINE_MUTEX(idr_lock); /* protects tty_idr */
+
+static struct tty_driver *rpmsg_tty_driver;
+
+struct rpmsg_tty_port {
+ struct tty_port port; /* TTY port data */
+ unsigned int id; /* TTY rpmsg index */
+ spinlock_t rx_lock; /* message reception lock */
+ struct rpmsg_device *rpdev; /* rpmsg device */
+};
+
+static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len,
+ void *priv, u32 src)
+{
+ int space;
+ unsigned char *cbuf;
+ struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
+
+ dev_dbg(&rpdev->dev, "msg(<- src 0x%x) len %d\n", src, len);
+
+ print_hex_dump_debug(__func__, DUMP_PREFIX_NONE, 16, 1, data, len,
+ true);
+
+ /* Flush the recv-ed none-zero data to tty node */
+ if (!len)
+ return -EINVAL;
+
+ spin_lock(&cport->rx_lock);
+ space = tty_prepare_flip_string(&cport->port, &cbuf, len);
+ if (space <= 0) {
+ dev_err(&rpdev->dev, "No memory for tty_prepare_flip_string\n");
+ spin_unlock(&cport->rx_lock);
+ return -ENOMEM;
+ }
+
+ if (space != len)
+ dev_warn(&rpdev->dev, "Trunc buffer from %d to %d\n",
+ len, space);
+
+ memcpy(cbuf, data, space);
+ tty_flip_buffer_push(&cport->port);
+ spin_unlock(&cport->rx_lock);
+
+ return 0;
+}
+
+static int rpmsg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
+{
+ struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
+
+ if (!cport) {
+ dev_err(tty->dev, "cannot get cport\n");
+ return -ENODEV;
+ }
+
+ return tty_port_install(&cport->port, driver, tty);
+}
+
+static int rpmsg_tty_open(struct tty_struct *tty, struct file *filp)
+{
+ return tty_port_open(tty->port, tty, filp);
+}
+
+static void rpmsg_tty_close(struct tty_struct *tty, struct file *filp)
+{
+ return tty_port_close(tty->port, tty, filp);
+}
+
+static int rpmsg_tty_write(struct tty_struct *tty, const unsigned char *buf,
+ int total)
+{
+ int count, ret = 0;
+ const unsigned char *tbuf;
+ struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
+ struct rpmsg_device *rpdev;
+ int msg_size;
+
+ if (!cport) {
+ dev_err(tty->dev, "cannot get cport\n");
+ return -ENODEV;
+ }
+
+ rpdev = cport->rpdev;
+
+ dev_dbg(&rpdev->dev, "%s: send message from tty->index = %d\n",
+ __func__, tty->index);
+
+ if (!buf) {
+ dev_err(&rpdev->dev, "buf shouldn't be null.\n");
+ return -ENOMEM;
+ }
+
+ msg_size = rpmsg_get_buf_payload_size(rpdev->ept);
+ if (msg_size < 0)
+ return msg_size;
+
+ count = total;
+ tbuf = buf;
+ do {
+ /* send a message to our remote processor */
+ ret = rpmsg_send(rpdev->ept, (void *)tbuf,
+ min(count, msg_size));
+ if (ret) {
+ dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
+ return ret;
+ }
+
+ if (count > msg_size) {
+ count -= msg_size;
+ tbuf += msg_size;
+ } else {
+ count = 0;
+ }
+ } while (count > 0);
+
+ return total;
+}
+
+static int rpmsg_tty_write_room(struct tty_struct *tty)
+{
+ struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
+
+ if (!cport) {
+ dev_err(tty->dev, "cannot get cport\n");
+ return -ENODEV;
+ }
+
+ /* report the space in the rpmsg buffer */
+ return rpmsg_get_buf_payload_size(cport->rpdev->ept);
+}
+
+static const struct tty_operations rpmsg_tty_ops = {
+ .install = rpmsg_tty_install,
+ .open = rpmsg_tty_open,
+ .close = rpmsg_tty_close,
+ .write = rpmsg_tty_write,
+ .write_room = rpmsg_tty_write_room,
+};
+
+struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
+{
+ struct rpmsg_tty_port *cport;
+
+ cport = kzalloc(sizeof(*cport), GFP_KERNEL);
+ if (!cport)
+ return ERR_PTR(-ENOMEM);
+
+ mutex_lock(&idr_lock);
+ cport->id = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
+ mutex_unlock(&idr_lock);
+
+ if (cport->id < 0) {
+ kfree(cport);
+ return ERR_PTR(-ENOSPC);
+ }
+
+ return cport;
+}
+
+static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport)
+{
+ mutex_lock(&idr_lock);
+ idr_remove(&tty_idr, cport->id);
+ mutex_unlock(&idr_lock);
+
+ kfree(cport);
+}
+
+static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
+{
+ struct rpmsg_tty_port *cport;
+ struct device *tty_dev;
+ int ret;
+
+ cport = rpmsg_tty_alloc_cport();
+ if (IS_ERR(cport)) {
+ dev_err(&rpdev->dev, "failed to alloc tty port\n");
+ return PTR_ERR(cport);
+ }
+
+ tty_port_init(&cport->port);
+ cport->port.low_latency = cport->port.flags | ASYNC_LOW_LATENCY;
+
+ tty_dev = tty_port_register_device(&cport->port, rpmsg_tty_driver,
+ cport->id, &rpdev->dev);
+ if (IS_ERR(tty_dev)) {
+ dev_err(&rpdev->dev, "failed to register tty port\n");
+ ret = PTR_ERR(tty_dev);
+ goto err_destroy;
+ }
+
+ spin_lock_init(&cport->rx_lock);
+ cport->rpdev = rpdev;
+
+ dev_set_drvdata(&rpdev->dev, cport);
+
+ dev_info(&rpdev->dev, "new channel: 0x%x -> 0x%x : ttyRPMSG%d\n",
+ rpdev->src, rpdev->dst, cport->id);
+
+ return 0;
+
+err_destroy:
+ tty_port_destroy(&cport->port);
+ rpmsg_tty_release_cport(cport);
+ return ret;
+}
+
+static void rpmsg_tty_remove(struct rpmsg_device *rpdev)
+{
+ struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
+
+ dev_info(&rpdev->dev, "removing rpmsg tty device %d\n", cport->id);
+
+ /* User hang up to release the tty */
+ if (tty_port_initialized(&cport->port))
+ tty_port_tty_hangup(&cport->port, false);
+
+ tty_unregister_device(rpmsg_tty_driver, cport->id);
+ tty_port_destroy(&cport->port);
+
+ rpmsg_tty_release_cport(cport);
+}
+
+static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
+ { .name = "rpmsg-tty-channel" },
+ { },
+};
+MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_tty_id_table);
+
+static struct rpmsg_driver rpmsg_tty_rpmsg_drv = {
+ .drv.name = KBUILD_MODNAME,
+ .id_table = rpmsg_driver_tty_id_table,
+ .probe = rpmsg_tty_probe,
+ .callback = rpmsg_tty_cb,
+ .remove = rpmsg_tty_remove,
+};
+
+static int __init rpmsg_tty_init(void)
+{
+ int err;
+
+ rpmsg_tty_driver = tty_alloc_driver(MAX_TTY_RPMSG, TTY_DRIVER_REAL_RAW |
+ TTY_DRIVER_DYNAMIC_DEV);
+ if (IS_ERR(rpmsg_tty_driver))
+ return PTR_ERR(rpmsg_tty_driver);
+
+ rpmsg_tty_driver->driver_name = "rpmsg_tty";
+ rpmsg_tty_driver->name = "ttyRPMSG";
+ rpmsg_tty_driver->major = 0;
+ rpmsg_tty_driver->type = TTY_DRIVER_TYPE_CONSOLE;
+ rpmsg_tty_driver->init_termios = tty_std_termios;
+
+ tty_set_operations(rpmsg_tty_driver, &rpmsg_tty_ops);
+
+ err = tty_register_driver(rpmsg_tty_driver);
+ if (err < 0) {
+ pr_err("Couldn't install rpmsg tty driver: err %d\n", err);
+ goto error_put;
+ }
+
+ err = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
+ if (err < 0) {
+ pr_err("Couldn't register rpmsg tty driver: err %d\n", err);
+ goto error_unregister;
+ }
+
+ return 0;
+
+error_unregister:
+ tty_unregister_driver(rpmsg_tty_driver);
+
+error_put:
+ put_tty_driver(rpmsg_tty_driver);
+
+ return err;
+}
+
+static void __exit rpmsg_tty_exit(void)
+{
+ unregister_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
+ tty_unregister_driver(rpmsg_tty_driver);
+ put_tty_driver(rpmsg_tty_driver);
+ idr_destroy(&tty_idr);
+}
+
+module_init(rpmsg_tty_init);
+module_exit(rpmsg_tty_exit);
+
+MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
+MODULE_AUTHOR("Fabien Dessenne <fabien.dessenne@st.com>");
+MODULE_DESCRIPTION("virtio remote processor messaging tty driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-03-21 15:47 ` [PATCH 2/2] tty: add rpmsg driver Fabien Dessenne
@ 2019-04-03 8:44 ` Johan Hovold
2019-04-05 12:50 ` Fabien DESSENNE
2019-04-03 12:47 ` xiang xiao
1 sibling, 1 reply; 19+ messages in thread
From: Johan Hovold @ 2019-04-03 8:44 UTC (permalink / raw)
To: Fabien Dessenne
Cc: Ohad Ben-Cohen, Bjorn Andersson, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel, linux-remoteproc, Benjamin Gaignard,
Arnaud Pouliquen
On Thu, Mar 21, 2019 at 04:47:19PM +0100, Fabien Dessenne 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@st.com>
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> ---
> drivers/tty/Kconfig | 9 ++
> drivers/tty/Makefile | 1 +
> drivers/tty/rpmsg_tty.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 319 insertions(+)
> create mode 100644 drivers/tty/rpmsg_tty.c
> +static int rpmsg_tty_write(struct tty_struct *tty, const unsigned char *buf,
> + int total)
> +{
> + int count, ret = 0;
> + const unsigned char *tbuf;
> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> + struct rpmsg_device *rpdev;
> + int msg_size;
> +
> + if (!cport) {
> + dev_err(tty->dev, "cannot get cport\n");
> + return -ENODEV;
> + }
> +
> + rpdev = cport->rpdev;
> +
> + dev_dbg(&rpdev->dev, "%s: send message from tty->index = %d\n",
> + __func__, tty->index);
> +
> + if (!buf) {
> + dev_err(&rpdev->dev, "buf shouldn't be null.\n");
> + return -ENOMEM;
> + }
> +
> + msg_size = rpmsg_get_buf_payload_size(rpdev->ept);
> + if (msg_size < 0)
> + return msg_size;
> +
> + count = total;
> + tbuf = buf;
> + do {
> + /* send a message to our remote processor */
> + ret = rpmsg_send(rpdev->ept, (void *)tbuf,
> + min(count, msg_size));
Just a drive-by comment; it looks like rpmsg_send() may block, but
the tty-driver write() callback must never sleep.
> + if (ret) {
> + dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
> + return ret;
> + }
> +
> + if (count > msg_size) {
> + count -= msg_size;
> + tbuf += msg_size;
> + } else {
> + count = 0;
> + }
> + } while (count > 0);
> +
> + return total;
> +}
Johan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-03-21 15:47 ` [PATCH 2/2] tty: add rpmsg driver Fabien Dessenne
2019-04-03 8:44 ` Johan Hovold
@ 2019-04-03 12:47 ` xiang xiao
2019-04-04 16:14 ` Arnaud Pouliquen
1 sibling, 1 reply; 19+ messages in thread
From: xiang xiao @ 2019-04-03 12:47 UTC (permalink / raw)
To: Fabien Dessenne
Cc: Ohad Ben-Cohen, Bjorn Andersson, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel, linux-remoteproc, Benjamin Gaignard,
Arnaud Pouliquen
On Thu, Mar 21, 2019 at 11:48 PM Fabien Dessenne <fabien.dessenne@st.com> 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.
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> ---
> drivers/tty/Kconfig | 9 ++
> drivers/tty/Makefile | 1 +
> drivers/tty/rpmsg_tty.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 319 insertions(+)
> create mode 100644 drivers/tty/rpmsg_tty.c
>
> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index 0840d27..42696e6 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -441,4 +441,13 @@ config VCC
> depends on SUN_LDOMS
> 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.
> endif # TTY
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index c72cafd..90a98a2 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
> new file mode 100644
> index 0000000..9c2a629
> --- /dev/null
> +++ b/drivers/tty/rpmsg_tty.c
> @@ -0,0 +1,309 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics 2019 - All Rights Reserved
> + * Authors: Arnaud Pouliquen <arnaud.pouliquen@st.com> for STMicroelectronics.
> + * Fabien Dessenne <fabien.dessenne@st.com> for STMicroelectronics.
> + * Derived from the imx-rpmsg and omap-rpmsg implementations.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/rpmsg.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +
> +#define MAX_TTY_RPMSG 32
> +
> +static DEFINE_IDR(tty_idr); /* tty instance id */
> +static DEFINE_MUTEX(idr_lock); /* protects tty_idr */
> +
> +static struct tty_driver *rpmsg_tty_driver;
> +
> +struct rpmsg_tty_port {
> + struct tty_port port; /* TTY port data */
> + unsigned int id; /* TTY rpmsg index */
> + spinlock_t rx_lock; /* message reception lock */
> + struct rpmsg_device *rpdev; /* rpmsg device */
> +};
> +
> +static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len,
> + void *priv, u32 src)
> +{
> + int space;
> + unsigned char *cbuf;
> + struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
> +
> + dev_dbg(&rpdev->dev, "msg(<- src 0x%x) len %d\n", src, len);
> +
> + print_hex_dump_debug(__func__, DUMP_PREFIX_NONE, 16, 1, data, len,
> + true);
> +
> + /* Flush the recv-ed none-zero data to tty node */
> + if (!len)
> + return -EINVAL;
> +
> + spin_lock(&cport->rx_lock);
> + space = tty_prepare_flip_string(&cport->port, &cbuf, len);
> + if (space <= 0) {
> + dev_err(&rpdev->dev, "No memory for tty_prepare_flip_string\n");
> + spin_unlock(&cport->rx_lock);
> + return -ENOMEM;
> + }
> +
> + if (space != len)
> + dev_warn(&rpdev->dev, "Trunc buffer from %d to %d\n",
> + len, space);
> +
> + memcpy(cbuf, data, space);
> + tty_flip_buffer_push(&cport->port);
> + spin_unlock(&cport->rx_lock);
> +
> + return 0;
> +}
> +
> +static int rpmsg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
> +{
> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> +
> + if (!cport) {
> + dev_err(tty->dev, "cannot get cport\n");
> + return -ENODEV;
> + }
> +
> + return tty_port_install(&cport->port, driver, tty);
> +}
> +
> +static int rpmsg_tty_open(struct tty_struct *tty, struct file *filp)
> +{
> + return tty_port_open(tty->port, tty, filp);
> +}
> +
> +static void rpmsg_tty_close(struct tty_struct *tty, struct file *filp)
> +{
> + return tty_port_close(tty->port, tty, filp);
> +}
> +
> +static int rpmsg_tty_write(struct tty_struct *tty, const unsigned char *buf,
> + int total)
> +{
> + int count, ret = 0;
> + const unsigned char *tbuf;
> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> + struct rpmsg_device *rpdev;
> + int msg_size;
> +
> + if (!cport) {
> + dev_err(tty->dev, "cannot get cport\n");
> + return -ENODEV;
> + }
> +
> + rpdev = cport->rpdev;
> +
> + dev_dbg(&rpdev->dev, "%s: send message from tty->index = %d\n",
> + __func__, tty->index);
> +
> + if (!buf) {
> + dev_err(&rpdev->dev, "buf shouldn't be null.\n");
> + return -ENOMEM;
> + }
> +
> + msg_size = rpmsg_get_buf_payload_size(rpdev->ept);
> + if (msg_size < 0)
> + return msg_size;
> +
> + count = total;
> + tbuf = buf;
> + do {
> + /* send a message to our remote processor */
> + ret = rpmsg_send(rpdev->ept, (void *)tbuf,
> + min(count, msg_size));
> + if (ret) {
> + dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
> + return ret;
> + }
> +
> + if (count > msg_size) {
> + count -= msg_size;
> + tbuf += msg_size;
> + } else {
> + count = 0;
> + }
> + } while (count > 0);
> +
We need the flow control(or handshake) here, otherwise it's very easy
to lose the data if kernel keep send the data as fast as possible.
> + return total;
> +}
> +
> +static int rpmsg_tty_write_room(struct tty_struct *tty)
> +{
> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> +
> + if (!cport) {
> + dev_err(tty->dev, "cannot get cport\n");
> + return -ENODEV;
> + }
> +
> + /* report the space in the rpmsg buffer */
> + return rpmsg_get_buf_payload_size(cport->rpdev->ept);
> +}
> +
> +static const struct tty_operations rpmsg_tty_ops = {
> + .install = rpmsg_tty_install,
> + .open = rpmsg_tty_open,
> + .close = rpmsg_tty_close,
> + .write = rpmsg_tty_write,
> + .write_room = rpmsg_tty_write_room,
> +};
> +
> +struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
> +{
> + struct rpmsg_tty_port *cport;
> +
> + cport = kzalloc(sizeof(*cport), GFP_KERNEL);
> + if (!cport)
> + return ERR_PTR(-ENOMEM);
> +
> + mutex_lock(&idr_lock);
> + cport->id = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
> + mutex_unlock(&idr_lock);
> +
> + if (cport->id < 0) {
> + kfree(cport);
> + return ERR_PTR(-ENOSPC);
> + }
> +
> + return cport;
> +}
> +
> +static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport)
> +{
> + mutex_lock(&idr_lock);
> + idr_remove(&tty_idr, cport->id);
> + mutex_unlock(&idr_lock);
> +
> + kfree(cport);
> +}
> +
> +static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
> +{
> + struct rpmsg_tty_port *cport;
> + struct device *tty_dev;
> + int ret;
> +
> + cport = rpmsg_tty_alloc_cport();
> + if (IS_ERR(cport)) {
> + dev_err(&rpdev->dev, "failed to alloc tty port\n");
> + return PTR_ERR(cport);
> + }
> +
> + tty_port_init(&cport->port);
> + cport->port.low_latency = cport->port.flags | ASYNC_LOW_LATENCY;
> +
> + tty_dev = tty_port_register_device(&cport->port, rpmsg_tty_driver,
> + cport->id, &rpdev->dev);
> + if (IS_ERR(tty_dev)) {
> + dev_err(&rpdev->dev, "failed to register tty port\n");
> + ret = PTR_ERR(tty_dev);
> + goto err_destroy;
> + }
> +
> + spin_lock_init(&cport->rx_lock);
> + cport->rpdev = rpdev;
> +
> + dev_set_drvdata(&rpdev->dev, cport);
> +
> + dev_info(&rpdev->dev, "new channel: 0x%x -> 0x%x : ttyRPMSG%d\n",
> + rpdev->src, rpdev->dst, cport->id);
> +
> + return 0;
> +
> +err_destroy:
> + tty_port_destroy(&cport->port);
> + rpmsg_tty_release_cport(cport);
> + return ret;
> +}
> +
> +static void rpmsg_tty_remove(struct rpmsg_device *rpdev)
> +{
> + struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
> +
> + dev_info(&rpdev->dev, "removing rpmsg tty device %d\n", cport->id);
> +
> + /* User hang up to release the tty */
> + if (tty_port_initialized(&cport->port))
> + tty_port_tty_hangup(&cport->port, false);
> +
> + tty_unregister_device(rpmsg_tty_driver, cport->id);
> + tty_port_destroy(&cport->port);
> +
> + rpmsg_tty_release_cport(cport);
> +}
> +
> +static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
> + { .name = "rpmsg-tty-channel" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_tty_id_table);
> +
> +static struct rpmsg_driver rpmsg_tty_rpmsg_drv = {
> + .drv.name = KBUILD_MODNAME,
> + .id_table = rpmsg_driver_tty_id_table,
> + .probe = rpmsg_tty_probe,
> + .callback = rpmsg_tty_cb,
> + .remove = rpmsg_tty_remove,
> +};
> +
> +static int __init rpmsg_tty_init(void)
> +{
> + int err;
> +
> + rpmsg_tty_driver = tty_alloc_driver(MAX_TTY_RPMSG, TTY_DRIVER_REAL_RAW |
> + TTY_DRIVER_DYNAMIC_DEV);
> + if (IS_ERR(rpmsg_tty_driver))
> + return PTR_ERR(rpmsg_tty_driver);
> +
> + rpmsg_tty_driver->driver_name = "rpmsg_tty";
> + rpmsg_tty_driver->name = "ttyRPMSG";
> + rpmsg_tty_driver->major = 0;
> + rpmsg_tty_driver->type = TTY_DRIVER_TYPE_CONSOLE;
> + rpmsg_tty_driver->init_termios = tty_std_termios;
> +
> + tty_set_operations(rpmsg_tty_driver, &rpmsg_tty_ops);
> +
> + err = tty_register_driver(rpmsg_tty_driver);
> + if (err < 0) {
> + pr_err("Couldn't install rpmsg tty driver: err %d\n", err);
> + goto error_put;
> + }
> +
> + err = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
> + if (err < 0) {
> + pr_err("Couldn't register rpmsg tty driver: err %d\n", err);
> + goto error_unregister;
> + }
> +
> + return 0;
> +
> +error_unregister:
> + tty_unregister_driver(rpmsg_tty_driver);
> +
> +error_put:
> + put_tty_driver(rpmsg_tty_driver);
> +
> + return err;
> +}
> +
> +static void __exit rpmsg_tty_exit(void)
> +{
> + unregister_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
> + tty_unregister_driver(rpmsg_tty_driver);
> + put_tty_driver(rpmsg_tty_driver);
> + idr_destroy(&tty_idr);
> +}
> +
> +module_init(rpmsg_tty_init);
> +module_exit(rpmsg_tty_exit);
> +
> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
> +MODULE_AUTHOR("Fabien Dessenne <fabien.dessenne@st.com>");
> +MODULE_DESCRIPTION("virtio remote processor messaging tty driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-04-03 12:47 ` xiang xiao
@ 2019-04-04 16:14 ` Arnaud Pouliquen
2019-04-05 10:12 ` xiang xiao
0 siblings, 1 reply; 19+ messages in thread
From: Arnaud Pouliquen @ 2019-04-04 16:14 UTC (permalink / raw)
To: xiang xiao, Fabien Dessenne
Cc: Ohad Ben-Cohen, Bjorn Andersson, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel, linux-remoteproc, Benjamin Gaignard
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@st.com> 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
using some wrapper functions available here:
https://github.com/STMicroelectronics/STM32CubeMP1/tree/master/Middlewares/Third_Party/OpenAMP
Best Regards
Arnaud
>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
>> ---
>> drivers/tty/Kconfig | 9 ++
>> drivers/tty/Makefile | 1 +
>> drivers/tty/rpmsg_tty.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 319 insertions(+)
>> create mode 100644 drivers/tty/rpmsg_tty.c
>>
>> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
>> index 0840d27..42696e6 100644
>> --- a/drivers/tty/Kconfig
>> +++ b/drivers/tty/Kconfig
>> @@ -441,4 +441,13 @@ config VCC
>> depends on SUN_LDOMS
>> 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.
>> endif # TTY
>> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
>> index c72cafd..90a98a2 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
>> new file mode 100644
>> index 0000000..9c2a629
>> --- /dev/null
>> +++ b/drivers/tty/rpmsg_tty.c
>> @@ -0,0 +1,309 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) STMicroelectronics 2019 - All Rights Reserved
>> + * Authors: Arnaud Pouliquen <arnaud.pouliquen@st.com> for STMicroelectronics.
>> + * Fabien Dessenne <fabien.dessenne@st.com> for STMicroelectronics.
>> + * Derived from the imx-rpmsg and omap-rpmsg implementations.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/rpmsg.h>
>> +#include <linux/slab.h>
>> +#include <linux/tty.h>
>> +#include <linux/tty_flip.h>
>> +
>> +#define MAX_TTY_RPMSG 32
>> +
>> +static DEFINE_IDR(tty_idr); /* tty instance id */
>> +static DEFINE_MUTEX(idr_lock); /* protects tty_idr */
>> +
>> +static struct tty_driver *rpmsg_tty_driver;
>> +
>> +struct rpmsg_tty_port {
>> + struct tty_port port; /* TTY port data */
>> + unsigned int id; /* TTY rpmsg index */
>> + spinlock_t rx_lock; /* message reception lock */
>> + struct rpmsg_device *rpdev; /* rpmsg device */
>> +};
>> +
>> +static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len,
>> + void *priv, u32 src)
>> +{
>> + int space;
>> + unsigned char *cbuf;
>> + struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
>> +
>> + dev_dbg(&rpdev->dev, "msg(<- src 0x%x) len %d\n", src, len);
>> +
>> + print_hex_dump_debug(__func__, DUMP_PREFIX_NONE, 16, 1, data, len,
>> + true);
>> +
>> + /* Flush the recv-ed none-zero data to tty node */
>> + if (!len)
>> + return -EINVAL;
>> +
>> + spin_lock(&cport->rx_lock);
>> + space = tty_prepare_flip_string(&cport->port, &cbuf, len);
>> + if (space <= 0) {
>> + dev_err(&rpdev->dev, "No memory for tty_prepare_flip_string\n");
>> + spin_unlock(&cport->rx_lock);
>> + return -ENOMEM;
>> + }
>> +
>> + if (space != len)
>> + dev_warn(&rpdev->dev, "Trunc buffer from %d to %d\n",
>> + len, space);
>> +
>> + memcpy(cbuf, data, space);
>> + tty_flip_buffer_push(&cport->port);
>> + spin_unlock(&cport->rx_lock);
>> +
>> + return 0;
>> +}
>> +
>> +static int rpmsg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
>> +{
>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
>> +
>> + if (!cport) {
>> + dev_err(tty->dev, "cannot get cport\n");
>> + return -ENODEV;
>> + }
>> +
>> + return tty_port_install(&cport->port, driver, tty);
>> +}
>> +
>> +static int rpmsg_tty_open(struct tty_struct *tty, struct file *filp)
>> +{
>> + return tty_port_open(tty->port, tty, filp);
>> +}
>> +
>> +static void rpmsg_tty_close(struct tty_struct *tty, struct file *filp)
>> +{
>> + return tty_port_close(tty->port, tty, filp);
>> +}
>> +
>> +static int rpmsg_tty_write(struct tty_struct *tty, const unsigned char *buf,
>> + int total)
>> +{
>> + int count, ret = 0;
>> + const unsigned char *tbuf;
>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
>> + struct rpmsg_device *rpdev;
>> + int msg_size;
>> +
>> + if (!cport) {
>> + dev_err(tty->dev, "cannot get cport\n");
>> + return -ENODEV;
>> + }
>> +
>> + rpdev = cport->rpdev;
>> +
>> + dev_dbg(&rpdev->dev, "%s: send message from tty->index = %d\n",
>> + __func__, tty->index);
>> +
>> + if (!buf) {
>> + dev_err(&rpdev->dev, "buf shouldn't be null.\n");
>> + return -ENOMEM;
>> + }
>> +
>> + msg_size = rpmsg_get_buf_payload_size(rpdev->ept);
>> + if (msg_size < 0)
>> + return msg_size;
>> +
>> + count = total;
>> + tbuf = buf;
>> + do {
>> + /* send a message to our remote processor */
>> + ret = rpmsg_send(rpdev->ept, (void *)tbuf,
>> + min(count, msg_size));
>> + if (ret) {
>> + dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + if (count > msg_size) {
>> + count -= msg_size;
>> + tbuf += msg_size;
>> + } else {
>> + count = 0;
>> + }
>> + } while (count > 0);
>> +
>
> We need the flow control(or handshake) here, otherwise it's very easy
> to lose the data if kernel keep send the data as fast as possible.
>
>> + return total;
>> +}
>> +
>> +static int rpmsg_tty_write_room(struct tty_struct *tty)
>> +{
>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
>> +
>> + if (!cport) {
>> + dev_err(tty->dev, "cannot get cport\n");
>> + return -ENODEV;
>> + }
>> +
>> + /* report the space in the rpmsg buffer */
>> + return rpmsg_get_buf_payload_size(cport->rpdev->ept);
>> +}
>> +
>> +static const struct tty_operations rpmsg_tty_ops = {
>> + .install = rpmsg_tty_install,
>> + .open = rpmsg_tty_open,
>> + .close = rpmsg_tty_close,
>> + .write = rpmsg_tty_write,
>> + .write_room = rpmsg_tty_write_room,
>> +};
>> +
>> +struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
>> +{
>> + struct rpmsg_tty_port *cport;
>> +
>> + cport = kzalloc(sizeof(*cport), GFP_KERNEL);
>> + if (!cport)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + mutex_lock(&idr_lock);
>> + cport->id = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
>> + mutex_unlock(&idr_lock);
>> +
>> + if (cport->id < 0) {
>> + kfree(cport);
>> + return ERR_PTR(-ENOSPC);
>> + }
>> +
>> + return cport;
>> +}
>> +
>> +static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport)
>> +{
>> + mutex_lock(&idr_lock);
>> + idr_remove(&tty_idr, cport->id);
>> + mutex_unlock(&idr_lock);
>> +
>> + kfree(cport);
>> +}
>> +
>> +static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
>> +{
>> + struct rpmsg_tty_port *cport;
>> + struct device *tty_dev;
>> + int ret;
>> +
>> + cport = rpmsg_tty_alloc_cport();
>> + if (IS_ERR(cport)) {
>> + dev_err(&rpdev->dev, "failed to alloc tty port\n");
>> + return PTR_ERR(cport);
>> + }
>> +
>> + tty_port_init(&cport->port);
>> + cport->port.low_latency = cport->port.flags | ASYNC_LOW_LATENCY;
>> +
>> + tty_dev = tty_port_register_device(&cport->port, rpmsg_tty_driver,
>> + cport->id, &rpdev->dev);
>> + if (IS_ERR(tty_dev)) {
>> + dev_err(&rpdev->dev, "failed to register tty port\n");
>> + ret = PTR_ERR(tty_dev);
>> + goto err_destroy;
>> + }
>> +
>> + spin_lock_init(&cport->rx_lock);
>> + cport->rpdev = rpdev;
>> +
>> + dev_set_drvdata(&rpdev->dev, cport);
>> +
>> + dev_info(&rpdev->dev, "new channel: 0x%x -> 0x%x : ttyRPMSG%d\n",
>> + rpdev->src, rpdev->dst, cport->id);
>> +
>> + return 0;
>> +
>> +err_destroy:
>> + tty_port_destroy(&cport->port);
>> + rpmsg_tty_release_cport(cport);
>> + return ret;
>> +}
>> +
>> +static void rpmsg_tty_remove(struct rpmsg_device *rpdev)
>> +{
>> + struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
>> +
>> + dev_info(&rpdev->dev, "removing rpmsg tty device %d\n", cport->id);
>> +
>> + /* User hang up to release the tty */
>> + if (tty_port_initialized(&cport->port))
>> + tty_port_tty_hangup(&cport->port, false);
>> +
>> + tty_unregister_device(rpmsg_tty_driver, cport->id);
>> + tty_port_destroy(&cport->port);
>> +
>> + rpmsg_tty_release_cport(cport);
>> +}
>> +
>> +static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
>> + { .name = "rpmsg-tty-channel" },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_tty_id_table);
>> +
>> +static struct rpmsg_driver rpmsg_tty_rpmsg_drv = {
>> + .drv.name = KBUILD_MODNAME,
>> + .id_table = rpmsg_driver_tty_id_table,
>> + .probe = rpmsg_tty_probe,
>> + .callback = rpmsg_tty_cb,
>> + .remove = rpmsg_tty_remove,
>> +};
>> +
>> +static int __init rpmsg_tty_init(void)
>> +{
>> + int err;
>> +
>> + rpmsg_tty_driver = tty_alloc_driver(MAX_TTY_RPMSG, TTY_DRIVER_REAL_RAW |
>> + TTY_DRIVER_DYNAMIC_DEV);
>> + if (IS_ERR(rpmsg_tty_driver))
>> + return PTR_ERR(rpmsg_tty_driver);
>> +
>> + rpmsg_tty_driver->driver_name = "rpmsg_tty";
>> + rpmsg_tty_driver->name = "ttyRPMSG";
>> + rpmsg_tty_driver->major = 0;
>> + rpmsg_tty_driver->type = TTY_DRIVER_TYPE_CONSOLE;
>> + rpmsg_tty_driver->init_termios = tty_std_termios;
>> +
>> + tty_set_operations(rpmsg_tty_driver, &rpmsg_tty_ops);
>> +
>> + err = tty_register_driver(rpmsg_tty_driver);
>> + if (err < 0) {
>> + pr_err("Couldn't install rpmsg tty driver: err %d\n", err);
>> + goto error_put;
>> + }
>> +
>> + err = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
>> + if (err < 0) {
>> + pr_err("Couldn't register rpmsg tty driver: err %d\n", err);
>> + goto error_unregister;
>> + }
>> +
>> + return 0;
>> +
>> +error_unregister:
>> + tty_unregister_driver(rpmsg_tty_driver);
>> +
>> +error_put:
>> + put_tty_driver(rpmsg_tty_driver);
>> +
>> + return err;
>> +}
>> +
>> +static void __exit rpmsg_tty_exit(void)
>> +{
>> + unregister_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
>> + tty_unregister_driver(rpmsg_tty_driver);
>> + put_tty_driver(rpmsg_tty_driver);
>> + idr_destroy(&tty_idr);
>> +}
>> +
>> +module_init(rpmsg_tty_init);
>> +module_exit(rpmsg_tty_exit);
>> +
>> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
>> +MODULE_AUTHOR("Fabien Dessenne <fabien.dessenne@st.com>");
>> +MODULE_DESCRIPTION("virtio remote processor messaging tty driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-04-04 16:14 ` Arnaud Pouliquen
@ 2019-04-05 10:12 ` xiang xiao
2019-04-05 12:33 ` Arnaud Pouliquen
0 siblings, 1 reply; 19+ messages in thread
From: xiang xiao @ 2019-04-05 10:12 UTC (permalink / raw)
To: Arnaud Pouliquen
Cc: Fabien Dessenne, Ohad Ben-Cohen, Bjorn Andersson,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-remoteproc,
Benjamin Gaignard
On Fri, Apr 5, 2019 at 12:14 AM Arnaud Pouliquen
<arnaud.pouliquen@st.com> 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@st.com> 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?
> using some wrapper functions available here:
> https://github.com/STMicroelectronics/STM32CubeMP1/tree/master/Middlewares/Third_Party/OpenAMP
>
>
> Best Regards
> Arnaud
>
> >
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> >> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> >> ---
> >> drivers/tty/Kconfig | 9 ++
> >> drivers/tty/Makefile | 1 +
> >> drivers/tty/rpmsg_tty.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 319 insertions(+)
> >> create mode 100644 drivers/tty/rpmsg_tty.c
> >>
> >> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> >> index 0840d27..42696e6 100644
> >> --- a/drivers/tty/Kconfig
> >> +++ b/drivers/tty/Kconfig
> >> @@ -441,4 +441,13 @@ config VCC
> >> depends on SUN_LDOMS
> >> 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.
> >> endif # TTY
> >> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> >> index c72cafd..90a98a2 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
> >> new file mode 100644
> >> index 0000000..9c2a629
> >> --- /dev/null
> >> +++ b/drivers/tty/rpmsg_tty.c
> >> @@ -0,0 +1,309 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) STMicroelectronics 2019 - All Rights Reserved
> >> + * Authors: Arnaud Pouliquen <arnaud.pouliquen@st.com> for STMicroelectronics.
> >> + * Fabien Dessenne <fabien.dessenne@st.com> for STMicroelectronics.
> >> + * Derived from the imx-rpmsg and omap-rpmsg implementations.
> >> + */
> >> +
> >> +#include <linux/module.h>
> >> +#include <linux/rpmsg.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/tty.h>
> >> +#include <linux/tty_flip.h>
> >> +
> >> +#define MAX_TTY_RPMSG 32
> >> +
> >> +static DEFINE_IDR(tty_idr); /* tty instance id */
> >> +static DEFINE_MUTEX(idr_lock); /* protects tty_idr */
> >> +
> >> +static struct tty_driver *rpmsg_tty_driver;
> >> +
> >> +struct rpmsg_tty_port {
> >> + struct tty_port port; /* TTY port data */
> >> + unsigned int id; /* TTY rpmsg index */
> >> + spinlock_t rx_lock; /* message reception lock */
> >> + struct rpmsg_device *rpdev; /* rpmsg device */
> >> +};
> >> +
> >> +static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len,
> >> + void *priv, u32 src)
> >> +{
> >> + int space;
> >> + unsigned char *cbuf;
> >> + struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
> >> +
> >> + dev_dbg(&rpdev->dev, "msg(<- src 0x%x) len %d\n", src, len);
> >> +
> >> + print_hex_dump_debug(__func__, DUMP_PREFIX_NONE, 16, 1, data, len,
> >> + true);
> >> +
> >> + /* Flush the recv-ed none-zero data to tty node */
> >> + if (!len)
> >> + return -EINVAL;
> >> +
> >> + spin_lock(&cport->rx_lock);
> >> + space = tty_prepare_flip_string(&cport->port, &cbuf, len);
> >> + if (space <= 0) {
> >> + dev_err(&rpdev->dev, "No memory for tty_prepare_flip_string\n");
> >> + spin_unlock(&cport->rx_lock);
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + if (space != len)
> >> + dev_warn(&rpdev->dev, "Trunc buffer from %d to %d\n",
> >> + len, space);
> >> +
> >> + memcpy(cbuf, data, space);
> >> + tty_flip_buffer_push(&cport->port);
> >> + spin_unlock(&cport->rx_lock);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int rpmsg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
> >> +{
> >> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> >> +
> >> + if (!cport) {
> >> + dev_err(tty->dev, "cannot get cport\n");
> >> + return -ENODEV;
> >> + }
> >> +
> >> + return tty_port_install(&cport->port, driver, tty);
> >> +}
> >> +
> >> +static int rpmsg_tty_open(struct tty_struct *tty, struct file *filp)
> >> +{
> >> + return tty_port_open(tty->port, tty, filp);
> >> +}
> >> +
> >> +static void rpmsg_tty_close(struct tty_struct *tty, struct file *filp)
> >> +{
> >> + return tty_port_close(tty->port, tty, filp);
> >> +}
> >> +
> >> +static int rpmsg_tty_write(struct tty_struct *tty, const unsigned char *buf,
> >> + int total)
> >> +{
> >> + int count, ret = 0;
> >> + const unsigned char *tbuf;
> >> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> >> + struct rpmsg_device *rpdev;
> >> + int msg_size;
> >> +
> >> + if (!cport) {
> >> + dev_err(tty->dev, "cannot get cport\n");
> >> + return -ENODEV;
> >> + }
> >> +
> >> + rpdev = cport->rpdev;
> >> +
> >> + dev_dbg(&rpdev->dev, "%s: send message from tty->index = %d\n",
> >> + __func__, tty->index);
> >> +
> >> + if (!buf) {
> >> + dev_err(&rpdev->dev, "buf shouldn't be null.\n");
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + msg_size = rpmsg_get_buf_payload_size(rpdev->ept);
> >> + if (msg_size < 0)
> >> + return msg_size;
> >> +
> >> + count = total;
> >> + tbuf = buf;
> >> + do {
> >> + /* send a message to our remote processor */
> >> + ret = rpmsg_send(rpdev->ept, (void *)tbuf,
> >> + min(count, msg_size));
> >> + if (ret) {
> >> + dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
> >> + return ret;
> >> + }
> >> +
> >> + if (count > msg_size) {
> >> + count -= msg_size;
> >> + tbuf += msg_size;
> >> + } else {
> >> + count = 0;
> >> + }
> >> + } while (count > 0);
> >> +
> >
> > We need the flow control(or handshake) here, otherwise it's very easy
> > to lose the data if kernel keep send the data as fast as possible.
> >
> >> + return total;
> >> +}
> >> +
> >> +static int rpmsg_tty_write_room(struct tty_struct *tty)
> >> +{
> >> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> >> +
> >> + if (!cport) {
> >> + dev_err(tty->dev, "cannot get cport\n");
> >> + return -ENODEV;
> >> + }
> >> +
> >> + /* report the space in the rpmsg buffer */
> >> + return rpmsg_get_buf_payload_size(cport->rpdev->ept);
> >> +}
> >> +
> >> +static const struct tty_operations rpmsg_tty_ops = {
> >> + .install = rpmsg_tty_install,
> >> + .open = rpmsg_tty_open,
> >> + .close = rpmsg_tty_close,
> >> + .write = rpmsg_tty_write,
> >> + .write_room = rpmsg_tty_write_room,
> >> +};
> >> +
> >> +struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
> >> +{
> >> + struct rpmsg_tty_port *cport;
> >> +
> >> + cport = kzalloc(sizeof(*cport), GFP_KERNEL);
> >> + if (!cport)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + mutex_lock(&idr_lock);
> >> + cport->id = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
> >> + mutex_unlock(&idr_lock);
> >> +
> >> + if (cport->id < 0) {
> >> + kfree(cport);
> >> + return ERR_PTR(-ENOSPC);
> >> + }
> >> +
> >> + return cport;
> >> +}
> >> +
> >> +static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport)
> >> +{
> >> + mutex_lock(&idr_lock);
> >> + idr_remove(&tty_idr, cport->id);
> >> + mutex_unlock(&idr_lock);
> >> +
> >> + kfree(cport);
> >> +}
> >> +
> >> +static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
> >> +{
> >> + struct rpmsg_tty_port *cport;
> >> + struct device *tty_dev;
> >> + int ret;
> >> +
> >> + cport = rpmsg_tty_alloc_cport();
> >> + if (IS_ERR(cport)) {
> >> + dev_err(&rpdev->dev, "failed to alloc tty port\n");
> >> + return PTR_ERR(cport);
> >> + }
> >> +
> >> + tty_port_init(&cport->port);
> >> + cport->port.low_latency = cport->port.flags | ASYNC_LOW_LATENCY;
> >> +
> >> + tty_dev = tty_port_register_device(&cport->port, rpmsg_tty_driver,
> >> + cport->id, &rpdev->dev);
> >> + if (IS_ERR(tty_dev)) {
> >> + dev_err(&rpdev->dev, "failed to register tty port\n");
> >> + ret = PTR_ERR(tty_dev);
> >> + goto err_destroy;
> >> + }
> >> +
> >> + spin_lock_init(&cport->rx_lock);
> >> + cport->rpdev = rpdev;
> >> +
> >> + dev_set_drvdata(&rpdev->dev, cport);
> >> +
> >> + dev_info(&rpdev->dev, "new channel: 0x%x -> 0x%x : ttyRPMSG%d\n",
> >> + rpdev->src, rpdev->dst, cport->id);
> >> +
> >> + return 0;
> >> +
> >> +err_destroy:
> >> + tty_port_destroy(&cport->port);
> >> + rpmsg_tty_release_cport(cport);
> >> + return ret;
> >> +}
> >> +
> >> +static void rpmsg_tty_remove(struct rpmsg_device *rpdev)
> >> +{
> >> + struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
> >> +
> >> + dev_info(&rpdev->dev, "removing rpmsg tty device %d\n", cport->id);
> >> +
> >> + /* User hang up to release the tty */
> >> + if (tty_port_initialized(&cport->port))
> >> + tty_port_tty_hangup(&cport->port, false);
> >> +
> >> + tty_unregister_device(rpmsg_tty_driver, cport->id);
> >> + tty_port_destroy(&cport->port);
> >> +
> >> + rpmsg_tty_release_cport(cport);
> >> +}
> >> +
> >> +static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
> >> + { .name = "rpmsg-tty-channel" },
> >> + { },
> >> +};
> >> +MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_tty_id_table);
> >> +
> >> +static struct rpmsg_driver rpmsg_tty_rpmsg_drv = {
> >> + .drv.name = KBUILD_MODNAME,
> >> + .id_table = rpmsg_driver_tty_id_table,
> >> + .probe = rpmsg_tty_probe,
> >> + .callback = rpmsg_tty_cb,
> >> + .remove = rpmsg_tty_remove,
> >> +};
> >> +
> >> +static int __init rpmsg_tty_init(void)
> >> +{
> >> + int err;
> >> +
> >> + rpmsg_tty_driver = tty_alloc_driver(MAX_TTY_RPMSG, TTY_DRIVER_REAL_RAW |
> >> + TTY_DRIVER_DYNAMIC_DEV);
> >> + if (IS_ERR(rpmsg_tty_driver))
> >> + return PTR_ERR(rpmsg_tty_driver);
> >> +
> >> + rpmsg_tty_driver->driver_name = "rpmsg_tty";
> >> + rpmsg_tty_driver->name = "ttyRPMSG";
> >> + rpmsg_tty_driver->major = 0;
> >> + rpmsg_tty_driver->type = TTY_DRIVER_TYPE_CONSOLE;
> >> + rpmsg_tty_driver->init_termios = tty_std_termios;
> >> +
> >> + tty_set_operations(rpmsg_tty_driver, &rpmsg_tty_ops);
> >> +
> >> + err = tty_register_driver(rpmsg_tty_driver);
> >> + if (err < 0) {
> >> + pr_err("Couldn't install rpmsg tty driver: err %d\n", err);
> >> + goto error_put;
> >> + }
> >> +
> >> + err = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
> >> + if (err < 0) {
> >> + pr_err("Couldn't register rpmsg tty driver: err %d\n", err);
> >> + goto error_unregister;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +error_unregister:
> >> + tty_unregister_driver(rpmsg_tty_driver);
> >> +
> >> +error_put:
> >> + put_tty_driver(rpmsg_tty_driver);
> >> +
> >> + return err;
> >> +}
> >> +
> >> +static void __exit rpmsg_tty_exit(void)
> >> +{
> >> + unregister_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
> >> + tty_unregister_driver(rpmsg_tty_driver);
> >> + put_tty_driver(rpmsg_tty_driver);
> >> + idr_destroy(&tty_idr);
> >> +}
> >> +
> >> +module_init(rpmsg_tty_init);
> >> +module_exit(rpmsg_tty_exit);
> >> +
> >> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
> >> +MODULE_AUTHOR("Fabien Dessenne <fabien.dessenne@st.com>");
> >> +MODULE_DESCRIPTION("virtio remote processor messaging tty driver");
> >> +MODULE_LICENSE("GPL v2");
> >> --
> >> 2.7.4
> >>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-04-05 10:12 ` xiang xiao
@ 2019-04-05 12:33 ` Arnaud Pouliquen
2019-04-05 14:03 ` xiang xiao
0 siblings, 1 reply; 19+ messages in thread
From: Arnaud Pouliquen @ 2019-04-05 12:33 UTC (permalink / raw)
To: xiang xiao
Cc: Fabien Dessenne, Ohad Ben-Cohen, Bjorn Andersson,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-remoteproc,
Benjamin Gaignard
On 4/5/19 12:12 PM, xiang xiao wrote:
> On Fri, Apr 5, 2019 at 12:14 AM Arnaud Pouliquen
> <arnaud.pouliquen@st.com> 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@st.com> 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.
If i well remember you have also a similar implementation of the service
on your side. Do you see any incompatibility with your implementation?
>
>> using some wrapper functions available here:
>> https://github.com/STMicroelectronics/STM32CubeMP1/tree/master/Middlewares/Third_Party/OpenAMP
>>
>>
>> Best Regards
>> Arnaud
>>
>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>>>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
>>>> ---
>>>> drivers/tty/Kconfig | 9 ++
>>>> drivers/tty/Makefile | 1 +
>>>> drivers/tty/rpmsg_tty.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 319 insertions(+)
>>>> create mode 100644 drivers/tty/rpmsg_tty.c
>>>>
>>>> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
>>>> index 0840d27..42696e6 100644
>>>> --- a/drivers/tty/Kconfig
>>>> +++ b/drivers/tty/Kconfig
>>>> @@ -441,4 +441,13 @@ config VCC
>>>> depends on SUN_LDOMS
>>>> 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.
>>>> endif # TTY
>>>> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
>>>> index c72cafd..90a98a2 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
>>>> new file mode 100644
>>>> index 0000000..9c2a629
>>>> --- /dev/null
>>>> +++ b/drivers/tty/rpmsg_tty.c
>>>> @@ -0,0 +1,309 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (C) STMicroelectronics 2019 - All Rights Reserved
>>>> + * Authors: Arnaud Pouliquen <arnaud.pouliquen@st.com> for STMicroelectronics.
>>>> + * Fabien Dessenne <fabien.dessenne@st.com> for STMicroelectronics.
>>>> + * Derived from the imx-rpmsg and omap-rpmsg implementations.
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/rpmsg.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/tty.h>
>>>> +#include <linux/tty_flip.h>
>>>> +
>>>> +#define MAX_TTY_RPMSG 32
>>>> +
>>>> +static DEFINE_IDR(tty_idr); /* tty instance id */
>>>> +static DEFINE_MUTEX(idr_lock); /* protects tty_idr */
>>>> +
>>>> +static struct tty_driver *rpmsg_tty_driver;
>>>> +
>>>> +struct rpmsg_tty_port {
>>>> + struct tty_port port; /* TTY port data */
>>>> + unsigned int id; /* TTY rpmsg index */
>>>> + spinlock_t rx_lock; /* message reception lock */
>>>> + struct rpmsg_device *rpdev; /* rpmsg device */
>>>> +};
>>>> +
>>>> +static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len,
>>>> + void *priv, u32 src)
>>>> +{
>>>> + int space;
>>>> + unsigned char *cbuf;
>>>> + struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
>>>> +
>>>> + dev_dbg(&rpdev->dev, "msg(<- src 0x%x) len %d\n", src, len);
>>>> +
>>>> + print_hex_dump_debug(__func__, DUMP_PREFIX_NONE, 16, 1, data, len,
>>>> + true);
>>>> +
>>>> + /* Flush the recv-ed none-zero data to tty node */
>>>> + if (!len)
>>>> + return -EINVAL;
>>>> +
>>>> + spin_lock(&cport->rx_lock);
>>>> + space = tty_prepare_flip_string(&cport->port, &cbuf, len);
>>>> + if (space <= 0) {
>>>> + dev_err(&rpdev->dev, "No memory for tty_prepare_flip_string\n");
>>>> + spin_unlock(&cport->rx_lock);
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + if (space != len)
>>>> + dev_warn(&rpdev->dev, "Trunc buffer from %d to %d\n",
>>>> + len, space);
>>>> +
>>>> + memcpy(cbuf, data, space);
>>>> + tty_flip_buffer_push(&cport->port);
>>>> + spin_unlock(&cport->rx_lock);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int rpmsg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
>>>> +{
>>>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
>>>> +
>>>> + if (!cport) {
>>>> + dev_err(tty->dev, "cannot get cport\n");
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + return tty_port_install(&cport->port, driver, tty);
>>>> +}
>>>> +
>>>> +static int rpmsg_tty_open(struct tty_struct *tty, struct file *filp)
>>>> +{
>>>> + return tty_port_open(tty->port, tty, filp);
>>>> +}
>>>> +
>>>> +static void rpmsg_tty_close(struct tty_struct *tty, struct file *filp)
>>>> +{
>>>> + return tty_port_close(tty->port, tty, filp);
>>>> +}
>>>> +
>>>> +static int rpmsg_tty_write(struct tty_struct *tty, const unsigned char *buf,
>>>> + int total)
>>>> +{
>>>> + int count, ret = 0;
>>>> + const unsigned char *tbuf;
>>>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
>>>> + struct rpmsg_device *rpdev;
>>>> + int msg_size;
>>>> +
>>>> + if (!cport) {
>>>> + dev_err(tty->dev, "cannot get cport\n");
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + rpdev = cport->rpdev;
>>>> +
>>>> + dev_dbg(&rpdev->dev, "%s: send message from tty->index = %d\n",
>>>> + __func__, tty->index);
>>>> +
>>>> + if (!buf) {
>>>> + dev_err(&rpdev->dev, "buf shouldn't be null.\n");
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + msg_size = rpmsg_get_buf_payload_size(rpdev->ept);
>>>> + if (msg_size < 0)
>>>> + return msg_size;
>>>> +
>>>> + count = total;
>>>> + tbuf = buf;
>>>> + do {
>>>> + /* send a message to our remote processor */
>>>> + ret = rpmsg_send(rpdev->ept, (void *)tbuf,
>>>> + min(count, msg_size));
>>>> + if (ret) {
>>>> + dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + if (count > msg_size) {
>>>> + count -= msg_size;
>>>> + tbuf += msg_size;
>>>> + } else {
>>>> + count = 0;
>>>> + }
>>>> + } while (count > 0);
>>>> +
>>>
>>> We need the flow control(or handshake) here, otherwise it's very easy
>>> to lose the data if kernel keep send the data as fast as possible.
>>>
>>>> + return total;
>>>> +}
>>>> +
>>>> +static int rpmsg_tty_write_room(struct tty_struct *tty)
>>>> +{
>>>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
>>>> +
>>>> + if (!cport) {
>>>> + dev_err(tty->dev, "cannot get cport\n");
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + /* report the space in the rpmsg buffer */
>>>> + return rpmsg_get_buf_payload_size(cport->rpdev->ept);
>>>> +}
>>>> +
>>>> +static const struct tty_operations rpmsg_tty_ops = {
>>>> + .install = rpmsg_tty_install,
>>>> + .open = rpmsg_tty_open,
>>>> + .close = rpmsg_tty_close,
>>>> + .write = rpmsg_tty_write,
>>>> + .write_room = rpmsg_tty_write_room,
>>>> +};
>>>> +
>>>> +struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
>>>> +{
>>>> + struct rpmsg_tty_port *cport;
>>>> +
>>>> + cport = kzalloc(sizeof(*cport), GFP_KERNEL);
>>>> + if (!cport)
>>>> + return ERR_PTR(-ENOMEM);
>>>> +
>>>> + mutex_lock(&idr_lock);
>>>> + cport->id = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
>>>> + mutex_unlock(&idr_lock);
>>>> +
>>>> + if (cport->id < 0) {
>>>> + kfree(cport);
>>>> + return ERR_PTR(-ENOSPC);
>>>> + }
>>>> +
>>>> + return cport;
>>>> +}
>>>> +
>>>> +static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport)
>>>> +{
>>>> + mutex_lock(&idr_lock);
>>>> + idr_remove(&tty_idr, cport->id);
>>>> + mutex_unlock(&idr_lock);
>>>> +
>>>> + kfree(cport);
>>>> +}
>>>> +
>>>> +static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
>>>> +{
>>>> + struct rpmsg_tty_port *cport;
>>>> + struct device *tty_dev;
>>>> + int ret;
>>>> +
>>>> + cport = rpmsg_tty_alloc_cport();
>>>> + if (IS_ERR(cport)) {
>>>> + dev_err(&rpdev->dev, "failed to alloc tty port\n");
>>>> + return PTR_ERR(cport);
>>>> + }
>>>> +
>>>> + tty_port_init(&cport->port);
>>>> + cport->port.low_latency = cport->port.flags | ASYNC_LOW_LATENCY;
>>>> +
>>>> + tty_dev = tty_port_register_device(&cport->port, rpmsg_tty_driver,
>>>> + cport->id, &rpdev->dev);
>>>> + if (IS_ERR(tty_dev)) {
>>>> + dev_err(&rpdev->dev, "failed to register tty port\n");
>>>> + ret = PTR_ERR(tty_dev);
>>>> + goto err_destroy;
>>>> + }
>>>> +
>>>> + spin_lock_init(&cport->rx_lock);
>>>> + cport->rpdev = rpdev;
>>>> +
>>>> + dev_set_drvdata(&rpdev->dev, cport);
>>>> +
>>>> + dev_info(&rpdev->dev, "new channel: 0x%x -> 0x%x : ttyRPMSG%d\n",
>>>> + rpdev->src, rpdev->dst, cport->id);
>>>> +
>>>> + return 0;
>>>> +
>>>> +err_destroy:
>>>> + tty_port_destroy(&cport->port);
>>>> + rpmsg_tty_release_cport(cport);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void rpmsg_tty_remove(struct rpmsg_device *rpdev)
>>>> +{
>>>> + struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
>>>> +
>>>> + dev_info(&rpdev->dev, "removing rpmsg tty device %d\n", cport->id);
>>>> +
>>>> + /* User hang up to release the tty */
>>>> + if (tty_port_initialized(&cport->port))
>>>> + tty_port_tty_hangup(&cport->port, false);
>>>> +
>>>> + tty_unregister_device(rpmsg_tty_driver, cport->id);
>>>> + tty_port_destroy(&cport->port);
>>>> +
>>>> + rpmsg_tty_release_cport(cport);
>>>> +}
>>>> +
>>>> +static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
>>>> + { .name = "rpmsg-tty-channel" },
>>>> + { },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_tty_id_table);
>>>> +
>>>> +static struct rpmsg_driver rpmsg_tty_rpmsg_drv = {
>>>> + .drv.name = KBUILD_MODNAME,
>>>> + .id_table = rpmsg_driver_tty_id_table,
>>>> + .probe = rpmsg_tty_probe,
>>>> + .callback = rpmsg_tty_cb,
>>>> + .remove = rpmsg_tty_remove,
>>>> +};
>>>> +
>>>> +static int __init rpmsg_tty_init(void)
>>>> +{
>>>> + int err;
>>>> +
>>>> + rpmsg_tty_driver = tty_alloc_driver(MAX_TTY_RPMSG, TTY_DRIVER_REAL_RAW |
>>>> + TTY_DRIVER_DYNAMIC_DEV);
>>>> + if (IS_ERR(rpmsg_tty_driver))
>>>> + return PTR_ERR(rpmsg_tty_driver);
>>>> +
>>>> + rpmsg_tty_driver->driver_name = "rpmsg_tty";
>>>> + rpmsg_tty_driver->name = "ttyRPMSG";
>>>> + rpmsg_tty_driver->major = 0;
>>>> + rpmsg_tty_driver->type = TTY_DRIVER_TYPE_CONSOLE;
>>>> + rpmsg_tty_driver->init_termios = tty_std_termios;
>>>> +
>>>> + tty_set_operations(rpmsg_tty_driver, &rpmsg_tty_ops);
>>>> +
>>>> + err = tty_register_driver(rpmsg_tty_driver);
>>>> + if (err < 0) {
>>>> + pr_err("Couldn't install rpmsg tty driver: err %d\n", err);
>>>> + goto error_put;
>>>> + }
>>>> +
>>>> + err = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
>>>> + if (err < 0) {
>>>> + pr_err("Couldn't register rpmsg tty driver: err %d\n", err);
>>>> + goto error_unregister;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +
>>>> +error_unregister:
>>>> + tty_unregister_driver(rpmsg_tty_driver);
>>>> +
>>>> +error_put:
>>>> + put_tty_driver(rpmsg_tty_driver);
>>>> +
>>>> + return err;
>>>> +}
>>>> +
>>>> +static void __exit rpmsg_tty_exit(void)
>>>> +{
>>>> + unregister_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
>>>> + tty_unregister_driver(rpmsg_tty_driver);
>>>> + put_tty_driver(rpmsg_tty_driver);
>>>> + idr_destroy(&tty_idr);
>>>> +}
>>>> +
>>>> +module_init(rpmsg_tty_init);
>>>> +module_exit(rpmsg_tty_exit);
>>>> +
>>>> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
>>>> +MODULE_AUTHOR("Fabien Dessenne <fabien.dessenne@st.com>");
>>>> +MODULE_DESCRIPTION("virtio remote processor messaging tty driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> --
>>>> 2.7.4
>>>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-04-03 8:44 ` Johan Hovold
@ 2019-04-05 12:50 ` Fabien DESSENNE
0 siblings, 0 replies; 19+ messages in thread
From: Fabien DESSENNE @ 2019-04-05 12:50 UTC (permalink / raw)
To: Johan Hovold
Cc: Ohad Ben-Cohen, Bjorn Andersson, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel, linux-remoteproc, Benjamin GAIGNARD,
Arnaud POULIQUEN
Hi Johan,
On 03/04/2019 10:44 AM, Johan Hovold wrote:
> On Thu, Mar 21, 2019 at 04:47:19PM +0100, Fabien Dessenne 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@st.com>
>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
>> ---
>> drivers/tty/Kconfig | 9 ++
>> drivers/tty/Makefile | 1 +
>> drivers/tty/rpmsg_tty.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 319 insertions(+)
>> create mode 100644 drivers/tty/rpmsg_tty.c
>> +static int rpmsg_tty_write(struct tty_struct *tty, const unsigned char *buf,
>> + int total)
>> +{
>> + int count, ret = 0;
>> + const unsigned char *tbuf;
>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
>> + struct rpmsg_device *rpdev;
>> + int msg_size;
>> +
>> + if (!cport) {
>> + dev_err(tty->dev, "cannot get cport\n");
>> + return -ENODEV;
>> + }
>> +
>> + rpdev = cport->rpdev;
>> +
>> + dev_dbg(&rpdev->dev, "%s: send message from tty->index = %d\n",
>> + __func__, tty->index);
>> +
>> + if (!buf) {
>> + dev_err(&rpdev->dev, "buf shouldn't be null.\n");
>> + return -ENOMEM;
>> + }
>> +
>> + msg_size = rpmsg_get_buf_payload_size(rpdev->ept);
>> + if (msg_size < 0)
>> + return msg_size;
>> +
>> + count = total;
>> + tbuf = buf;
>> + do {
>> + /* send a message to our remote processor */
>> + ret = rpmsg_send(rpdev->ept, (void *)tbuf,
>> + min(count, msg_size));
> Just a drive-by comment; it looks like rpmsg_send() may block, but
> the tty-driver write() callback must never sleep.
OK, I will use rpmsg_trysend() which does not block.
BR,
Fabien
>
>> + if (ret) {
>> + dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + if (count > msg_size) {
>> + count -= msg_size;
>> + tbuf += msg_size;
>> + } else {
>> + count = 0;
>> + }
>> + } while (count > 0);
>> +
>> + return total;
>> +}
> Johan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-04-05 12:33 ` Arnaud Pouliquen
@ 2019-04-05 14:03 ` xiang xiao
2019-04-05 16:08 ` Arnaud Pouliquen
0 siblings, 1 reply; 19+ messages in thread
From: xiang xiao @ 2019-04-05 14:03 UTC (permalink / raw)
To: Arnaud Pouliquen
Cc: Fabien Dessenne, Ohad Ben-Cohen, Bjorn Andersson,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-remoteproc,
Benjamin Gaignard
On Fri, Apr 5, 2019 at 8:33 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>
>
>
> On 4/5/19 12:12 PM, xiang xiao wrote:
> > On Fri, Apr 5, 2019 at 12:14 AM Arnaud Pouliquen
> > <arnaud.pouliquen@st.com> 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@st.com> 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
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).
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.
> >
> >> using some wrapper functions available here:
> >> https://github.com/STMicroelectronics/STM32CubeMP1/tree/master/Middlewares/Third_Party/OpenAMP
> >>
> >>
> >> Best Regards
> >> Arnaud
> >>
> >>>
> >>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> >>>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> >>>> ---
> >>>> drivers/tty/Kconfig | 9 ++
> >>>> drivers/tty/Makefile | 1 +
> >>>> drivers/tty/rpmsg_tty.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>> 3 files changed, 319 insertions(+)
> >>>> create mode 100644 drivers/tty/rpmsg_tty.c
> >>>>
> >>>> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> >>>> index 0840d27..42696e6 100644
> >>>> --- a/drivers/tty/Kconfig
> >>>> +++ b/drivers/tty/Kconfig
> >>>> @@ -441,4 +441,13 @@ config VCC
> >>>> depends on SUN_LDOMS
> >>>> 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.
> >>>> endif # TTY
> >>>> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> >>>> index c72cafd..90a98a2 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
> >>>> new file mode 100644
> >>>> index 0000000..9c2a629
> >>>> --- /dev/null
> >>>> +++ b/drivers/tty/rpmsg_tty.c
> >>>> @@ -0,0 +1,309 @@
> >>>> +// SPDX-License-Identifier: GPL-2.0
> >>>> +/*
> >>>> + * Copyright (C) STMicroelectronics 2019 - All Rights Reserved
> >>>> + * Authors: Arnaud Pouliquen <arnaud.pouliquen@st.com> for STMicroelectronics.
> >>>> + * Fabien Dessenne <fabien.dessenne@st.com> for STMicroelectronics.
> >>>> + * Derived from the imx-rpmsg and omap-rpmsg implementations.
> >>>> + */
> >>>> +
> >>>> +#include <linux/module.h>
> >>>> +#include <linux/rpmsg.h>
> >>>> +#include <linux/slab.h>
> >>>> +#include <linux/tty.h>
> >>>> +#include <linux/tty_flip.h>
> >>>> +
> >>>> +#define MAX_TTY_RPMSG 32
> >>>> +
> >>>> +static DEFINE_IDR(tty_idr); /* tty instance id */
> >>>> +static DEFINE_MUTEX(idr_lock); /* protects tty_idr */
> >>>> +
> >>>> +static struct tty_driver *rpmsg_tty_driver;
> >>>> +
> >>>> +struct rpmsg_tty_port {
> >>>> + struct tty_port port; /* TTY port data */
> >>>> + unsigned int id; /* TTY rpmsg index */
> >>>> + spinlock_t rx_lock; /* message reception lock */
> >>>> + struct rpmsg_device *rpdev; /* rpmsg device */
> >>>> +};
> >>>> +
> >>>> +static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len,
> >>>> + void *priv, u32 src)
> >>>> +{
> >>>> + int space;
> >>>> + unsigned char *cbuf;
> >>>> + struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
> >>>> +
> >>>> + dev_dbg(&rpdev->dev, "msg(<- src 0x%x) len %d\n", src, len);
> >>>> +
> >>>> + print_hex_dump_debug(__func__, DUMP_PREFIX_NONE, 16, 1, data, len,
> >>>> + true);
> >>>> +
> >>>> + /* Flush the recv-ed none-zero data to tty node */
> >>>> + if (!len)
> >>>> + return -EINVAL;
> >>>> +
> >>>> + spin_lock(&cport->rx_lock);
> >>>> + space = tty_prepare_flip_string(&cport->port, &cbuf, len);
> >>>> + if (space <= 0) {
> >>>> + dev_err(&rpdev->dev, "No memory for tty_prepare_flip_string\n");
> >>>> + spin_unlock(&cport->rx_lock);
> >>>> + return -ENOMEM;
> >>>> + }
> >>>> +
> >>>> + if (space != len)
> >>>> + dev_warn(&rpdev->dev, "Trunc buffer from %d to %d\n",
> >>>> + len, space);
> >>>> +
> >>>> + memcpy(cbuf, data, space);
> >>>> + tty_flip_buffer_push(&cport->port);
> >>>> + spin_unlock(&cport->rx_lock);
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static int rpmsg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
> >>>> +{
> >>>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> >>>> +
> >>>> + if (!cport) {
> >>>> + dev_err(tty->dev, "cannot get cport\n");
> >>>> + return -ENODEV;
> >>>> + }
> >>>> +
> >>>> + return tty_port_install(&cport->port, driver, tty);
> >>>> +}
> >>>> +
> >>>> +static int rpmsg_tty_open(struct tty_struct *tty, struct file *filp)
> >>>> +{
> >>>> + return tty_port_open(tty->port, tty, filp);
> >>>> +}
> >>>> +
> >>>> +static void rpmsg_tty_close(struct tty_struct *tty, struct file *filp)
> >>>> +{
> >>>> + return tty_port_close(tty->port, tty, filp);
> >>>> +}
> >>>> +
> >>>> +static int rpmsg_tty_write(struct tty_struct *tty, const unsigned char *buf,
> >>>> + int total)
> >>>> +{
> >>>> + int count, ret = 0;
> >>>> + const unsigned char *tbuf;
> >>>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> >>>> + struct rpmsg_device *rpdev;
> >>>> + int msg_size;
> >>>> +
> >>>> + if (!cport) {
> >>>> + dev_err(tty->dev, "cannot get cport\n");
> >>>> + return -ENODEV;
> >>>> + }
> >>>> +
> >>>> + rpdev = cport->rpdev;
> >>>> +
> >>>> + dev_dbg(&rpdev->dev, "%s: send message from tty->index = %d\n",
> >>>> + __func__, tty->index);
> >>>> +
> >>>> + if (!buf) {
> >>>> + dev_err(&rpdev->dev, "buf shouldn't be null.\n");
> >>>> + return -ENOMEM;
> >>>> + }
> >>>> +
> >>>> + msg_size = rpmsg_get_buf_payload_size(rpdev->ept);
> >>>> + if (msg_size < 0)
> >>>> + return msg_size;
> >>>> +
> >>>> + count = total;
> >>>> + tbuf = buf;
> >>>> + do {
> >>>> + /* send a message to our remote processor */
> >>>> + ret = rpmsg_send(rpdev->ept, (void *)tbuf,
> >>>> + min(count, msg_size));
> >>>> + if (ret) {
> >>>> + dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
> >>>> + return ret;
> >>>> + }
> >>>> +
> >>>> + if (count > msg_size) {
> >>>> + count -= msg_size;
> >>>> + tbuf += msg_size;
> >>>> + } else {
> >>>> + count = 0;
> >>>> + }
> >>>> + } while (count > 0);
> >>>> +
> >>>
> >>> We need the flow control(or handshake) here, otherwise it's very easy
> >>> to lose the data if kernel keep send the data as fast as possible.
> >>>
> >>>> + return total;
> >>>> +}
> >>>> +
> >>>> +static int rpmsg_tty_write_room(struct tty_struct *tty)
> >>>> +{
> >>>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> >>>> +
> >>>> + if (!cport) {
> >>>> + dev_err(tty->dev, "cannot get cport\n");
> >>>> + return -ENODEV;
> >>>> + }
> >>>> +
> >>>> + /* report the space in the rpmsg buffer */
> >>>> + return rpmsg_get_buf_payload_size(cport->rpdev->ept);
> >>>> +}
> >>>> +
> >>>> +static const struct tty_operations rpmsg_tty_ops = {
> >>>> + .install = rpmsg_tty_install,
> >>>> + .open = rpmsg_tty_open,
> >>>> + .close = rpmsg_tty_close,
> >>>> + .write = rpmsg_tty_write,
> >>>> + .write_room = rpmsg_tty_write_room,
> >>>> +};
> >>>> +
> >>>> +struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
> >>>> +{
> >>>> + struct rpmsg_tty_port *cport;
> >>>> +
> >>>> + cport = kzalloc(sizeof(*cport), GFP_KERNEL);
> >>>> + if (!cport)
> >>>> + return ERR_PTR(-ENOMEM);
> >>>> +
> >>>> + mutex_lock(&idr_lock);
> >>>> + cport->id = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
> >>>> + mutex_unlock(&idr_lock);
> >>>> +
> >>>> + if (cport->id < 0) {
> >>>> + kfree(cport);
> >>>> + return ERR_PTR(-ENOSPC);
> >>>> + }
> >>>> +
> >>>> + return cport;
> >>>> +}
> >>>> +
> >>>> +static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport)
> >>>> +{
> >>>> + mutex_lock(&idr_lock);
> >>>> + idr_remove(&tty_idr, cport->id);
> >>>> + mutex_unlock(&idr_lock);
> >>>> +
> >>>> + kfree(cport);
> >>>> +}
> >>>> +
> >>>> +static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
> >>>> +{
> >>>> + struct rpmsg_tty_port *cport;
> >>>> + struct device *tty_dev;
> >>>> + int ret;
> >>>> +
> >>>> + cport = rpmsg_tty_alloc_cport();
> >>>> + if (IS_ERR(cport)) {
> >>>> + dev_err(&rpdev->dev, "failed to alloc tty port\n");
> >>>> + return PTR_ERR(cport);
> >>>> + }
> >>>> +
> >>>> + tty_port_init(&cport->port);
> >>>> + cport->port.low_latency = cport->port.flags | ASYNC_LOW_LATENCY;
> >>>> +
> >>>> + tty_dev = tty_port_register_device(&cport->port, rpmsg_tty_driver,
> >>>> + cport->id, &rpdev->dev);
> >>>> + if (IS_ERR(tty_dev)) {
> >>>> + dev_err(&rpdev->dev, "failed to register tty port\n");
> >>>> + ret = PTR_ERR(tty_dev);
> >>>> + goto err_destroy;
> >>>> + }
> >>>> +
> >>>> + spin_lock_init(&cport->rx_lock);
> >>>> + cport->rpdev = rpdev;
> >>>> +
> >>>> + dev_set_drvdata(&rpdev->dev, cport);
> >>>> +
> >>>> + dev_info(&rpdev->dev, "new channel: 0x%x -> 0x%x : ttyRPMSG%d\n",
> >>>> + rpdev->src, rpdev->dst, cport->id);
> >>>> +
> >>>> + return 0;
> >>>> +
> >>>> +err_destroy:
> >>>> + tty_port_destroy(&cport->port);
> >>>> + rpmsg_tty_release_cport(cport);
> >>>> + return ret;
> >>>> +}
> >>>> +
> >>>> +static void rpmsg_tty_remove(struct rpmsg_device *rpdev)
> >>>> +{
> >>>> + struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
> >>>> +
> >>>> + dev_info(&rpdev->dev, "removing rpmsg tty device %d\n", cport->id);
> >>>> +
> >>>> + /* User hang up to release the tty */
> >>>> + if (tty_port_initialized(&cport->port))
> >>>> + tty_port_tty_hangup(&cport->port, false);
> >>>> +
> >>>> + tty_unregister_device(rpmsg_tty_driver, cport->id);
> >>>> + tty_port_destroy(&cport->port);
> >>>> +
> >>>> + rpmsg_tty_release_cport(cport);
> >>>> +}
> >>>> +
> >>>> +static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
> >>>> + { .name = "rpmsg-tty-channel" },
> >>>> + { },
> >>>> +};
> >>>> +MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_tty_id_table);
> >>>> +
> >>>> +static struct rpmsg_driver rpmsg_tty_rpmsg_drv = {
> >>>> + .drv.name = KBUILD_MODNAME,
> >>>> + .id_table = rpmsg_driver_tty_id_table,
> >>>> + .probe = rpmsg_tty_probe,
> >>>> + .callback = rpmsg_tty_cb,
> >>>> + .remove = rpmsg_tty_remove,
> >>>> +};
> >>>> +
> >>>> +static int __init rpmsg_tty_init(void)
> >>>> +{
> >>>> + int err;
> >>>> +
> >>>> + rpmsg_tty_driver = tty_alloc_driver(MAX_TTY_RPMSG, TTY_DRIVER_REAL_RAW |
> >>>> + TTY_DRIVER_DYNAMIC_DEV);
> >>>> + if (IS_ERR(rpmsg_tty_driver))
> >>>> + return PTR_ERR(rpmsg_tty_driver);
> >>>> +
> >>>> + rpmsg_tty_driver->driver_name = "rpmsg_tty";
> >>>> + rpmsg_tty_driver->name = "ttyRPMSG";
> >>>> + rpmsg_tty_driver->major = 0;
> >>>> + rpmsg_tty_driver->type = TTY_DRIVER_TYPE_CONSOLE;
> >>>> + rpmsg_tty_driver->init_termios = tty_std_termios;
> >>>> +
> >>>> + tty_set_operations(rpmsg_tty_driver, &rpmsg_tty_ops);
> >>>> +
> >>>> + err = tty_register_driver(rpmsg_tty_driver);
> >>>> + if (err < 0) {
> >>>> + pr_err("Couldn't install rpmsg tty driver: err %d\n", err);
> >>>> + goto error_put;
> >>>> + }
> >>>> +
> >>>> + err = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
> >>>> + if (err < 0) {
> >>>> + pr_err("Couldn't register rpmsg tty driver: err %d\n", err);
> >>>> + goto error_unregister;
> >>>> + }
> >>>> +
> >>>> + return 0;
> >>>> +
> >>>> +error_unregister:
> >>>> + tty_unregister_driver(rpmsg_tty_driver);
> >>>> +
> >>>> +error_put:
> >>>> + put_tty_driver(rpmsg_tty_driver);
> >>>> +
> >>>> + return err;
> >>>> +}
> >>>> +
> >>>> +static void __exit rpmsg_tty_exit(void)
> >>>> +{
> >>>> + unregister_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
> >>>> + tty_unregister_driver(rpmsg_tty_driver);
> >>>> + put_tty_driver(rpmsg_tty_driver);
> >>>> + idr_destroy(&tty_idr);
> >>>> +}
> >>>> +
> >>>> +module_init(rpmsg_tty_init);
> >>>> +module_exit(rpmsg_tty_exit);
> >>>> +
> >>>> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
> >>>> +MODULE_AUTHOR("Fabien Dessenne <fabien.dessenne@st.com>");
> >>>> +MODULE_DESCRIPTION("virtio remote processor messaging tty driver");
> >>>> +MODULE_LICENSE("GPL v2");
> >>>> --
> >>>> 2.7.4
> >>>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-04-05 14:03 ` xiang xiao
@ 2019-04-05 16:08 ` Arnaud Pouliquen
2019-04-06 7:56 ` xiang xiao
0 siblings, 1 reply; 19+ messages in thread
From: Arnaud Pouliquen @ 2019-04-05 16:08 UTC (permalink / raw)
To: xiang xiao
Cc: Fabien Dessenne, Ohad Ben-Cohen, Bjorn Andersson,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-remoteproc,
Benjamin Gaignard
On 4/5/19 4:03 PM, xiang xiao wrote:
> On Fri, Apr 5, 2019 at 8:33 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>>
>>
>>
>> On 4/5/19 12:12 PM, xiang xiao wrote:
>>> On Fri, Apr 5, 2019 at 12:14 AM Arnaud Pouliquen
>>> <arnaud.pouliquen@st.com> 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@st.com> 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...
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?
> 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?
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."
Why no just let rpmsg should manage the case with no Tx buffer free,
with a rpmsg_trysend...
>
> 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.
>
>>>
>>>> using some wrapper functions available here:
>>>> https://github.com/STMicroelectronics/STM32CubeMP1/tree/master/Middlewares/Third_Party/OpenAMP
>>>>
>>>>
>>>> Best Regards
>>>> Arnaud
>>>>
>>>>>
>>>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>>>>>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
>>>>>> ---
>>>>>> drivers/tty/Kconfig | 9 ++
>>>>>> drivers/tty/Makefile | 1 +
>>>>>> drivers/tty/rpmsg_tty.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> 3 files changed, 319 insertions(+)
>>>>>> create mode 100644 drivers/tty/rpmsg_tty.c
>>>>>>
>>>>>> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
>>>>>> index 0840d27..42696e6 100644
>>>>>> --- a/drivers/tty/Kconfig
>>>>>> +++ b/drivers/tty/Kconfig
>>>>>> @@ -441,4 +441,13 @@ config VCC
>>>>>> depends on SUN_LDOMS
>>>>>> 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.
>>>>>> endif # TTY
>>>>>> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
>>>>>> index c72cafd..90a98a2 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
>>>>>> new file mode 100644
>>>>>> index 0000000..9c2a629
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/tty/rpmsg_tty.c
>>>>>> @@ -0,0 +1,309 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +/*
>>>>>> + * Copyright (C) STMicroelectronics 2019 - All Rights Reserved
>>>>>> + * Authors: Arnaud Pouliquen <arnaud.pouliquen@st.com> for STMicroelectronics.
>>>>>> + * Fabien Dessenne <fabien.dessenne@st.com> for STMicroelectronics.
>>>>>> + * Derived from the imx-rpmsg and omap-rpmsg implementations.
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/module.h>
>>>>>> +#include <linux/rpmsg.h>
>>>>>> +#include <linux/slab.h>
>>>>>> +#include <linux/tty.h>
>>>>>> +#include <linux/tty_flip.h>
>>>>>> +
>>>>>> +#define MAX_TTY_RPMSG 32
>>>>>> +
>>>>>> +static DEFINE_IDR(tty_idr); /* tty instance id */
>>>>>> +static DEFINE_MUTEX(idr_lock); /* protects tty_idr */
>>>>>> +
>>>>>> +static struct tty_driver *rpmsg_tty_driver;
>>>>>> +
>>>>>> +struct rpmsg_tty_port {
>>>>>> + struct tty_port port; /* TTY port data */
>>>>>> + unsigned int id; /* TTY rpmsg index */
>>>>>> + spinlock_t rx_lock; /* message reception lock */
>>>>>> + struct rpmsg_device *rpdev; /* rpmsg device */
>>>>>> +};
>>>>>> +
>>>>>> +static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len,
>>>>>> + void *priv, u32 src)
>>>>>> +{
>>>>>> + int space;
>>>>>> + unsigned char *cbuf;
>>>>>> + struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
>>>>>> +
>>>>>> + dev_dbg(&rpdev->dev, "msg(<- src 0x%x) len %d\n", src, len);
>>>>>> +
>>>>>> + print_hex_dump_debug(__func__, DUMP_PREFIX_NONE, 16, 1, data, len,
>>>>>> + true);
>>>>>> +
>>>>>> + /* Flush the recv-ed none-zero data to tty node */
>>>>>> + if (!len)
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + spin_lock(&cport->rx_lock);
>>>>>> + space = tty_prepare_flip_string(&cport->port, &cbuf, len);
>>>>>> + if (space <= 0) {
>>>>>> + dev_err(&rpdev->dev, "No memory for tty_prepare_flip_string\n");
>>>>>> + spin_unlock(&cport->rx_lock);
>>>>>> + return -ENOMEM;
>>>>>> + }
>>>>>> +
>>>>>> + if (space != len)
>>>>>> + dev_warn(&rpdev->dev, "Trunc buffer from %d to %d\n",
>>>>>> + len, space);
>>>>>> +
>>>>>> + memcpy(cbuf, data, space);
>>>>>> + tty_flip_buffer_push(&cport->port);
>>>>>> + spin_unlock(&cport->rx_lock);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int rpmsg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
>>>>>> +{
>>>>>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
>>>>>> +
>>>>>> + if (!cport) {
>>>>>> + dev_err(tty->dev, "cannot get cport\n");
>>>>>> + return -ENODEV;
>>>>>> + }
>>>>>> +
>>>>>> + return tty_port_install(&cport->port, driver, tty);
>>>>>> +}
>>>>>> +
>>>>>> +static int rpmsg_tty_open(struct tty_struct *tty, struct file *filp)
>>>>>> +{
>>>>>> + return tty_port_open(tty->port, tty, filp);
>>>>>> +}
>>>>>> +
>>>>>> +static void rpmsg_tty_close(struct tty_struct *tty, struct file *filp)
>>>>>> +{
>>>>>> + return tty_port_close(tty->port, tty, filp);
>>>>>> +}
>>>>>> +
>>>>>> +static int rpmsg_tty_write(struct tty_struct *tty, const unsigned char *buf,
>>>>>> + int total)
>>>>>> +{
>>>>>> + int count, ret = 0;
>>>>>> + const unsigned char *tbuf;
>>>>>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
>>>>>> + struct rpmsg_device *rpdev;
>>>>>> + int msg_size;
>>>>>> +
>>>>>> + if (!cport) {
>>>>>> + dev_err(tty->dev, "cannot get cport\n");
>>>>>> + return -ENODEV;
>>>>>> + }
>>>>>> +
>>>>>> + rpdev = cport->rpdev;
>>>>>> +
>>>>>> + dev_dbg(&rpdev->dev, "%s: send message from tty->index = %d\n",
>>>>>> + __func__, tty->index);
>>>>>> +
>>>>>> + if (!buf) {
>>>>>> + dev_err(&rpdev->dev, "buf shouldn't be null.\n");
>>>>>> + return -ENOMEM;
>>>>>> + }
>>>>>> +
>>>>>> + msg_size = rpmsg_get_buf_payload_size(rpdev->ept);
>>>>>> + if (msg_size < 0)
>>>>>> + return msg_size;
>>>>>> +
>>>>>> + count = total;
>>>>>> + tbuf = buf;
>>>>>> + do {
>>>>>> + /* send a message to our remote processor */
>>>>>> + ret = rpmsg_send(rpdev->ept, (void *)tbuf,
>>>>>> + min(count, msg_size));
>>>>>> + if (ret) {
>>>>>> + dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> + if (count > msg_size) {
>>>>>> + count -= msg_size;
>>>>>> + tbuf += msg_size;
>>>>>> + } else {
>>>>>> + count = 0;
>>>>>> + }
>>>>>> + } while (count > 0);
>>>>>> +
>>>>>
>>>>> We need the flow control(or handshake) here, otherwise it's very easy
>>>>> to lose the data if kernel keep send the data as fast as possible.
>>>>>
>>>>>> + return total;
>>>>>> +}
>>>>>> +
>>>>>> +static int rpmsg_tty_write_room(struct tty_struct *tty)
>>>>>> +{
>>>>>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
>>>>>> +
>>>>>> + if (!cport) {
>>>>>> + dev_err(tty->dev, "cannot get cport\n");
>>>>>> + return -ENODEV;
>>>>>> + }
>>>>>> +
>>>>>> + /* report the space in the rpmsg buffer */
>>>>>> + return rpmsg_get_buf_payload_size(cport->rpdev->ept);
>>>>>> +}
>>>>>> +
>>>>>> +static const struct tty_operations rpmsg_tty_ops = {
>>>>>> + .install = rpmsg_tty_install,
>>>>>> + .open = rpmsg_tty_open,
>>>>>> + .close = rpmsg_tty_close,
>>>>>> + .write = rpmsg_tty_write,
>>>>>> + .write_room = rpmsg_tty_write_room,
>>>>>> +};
>>>>>> +
>>>>>> +struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
>>>>>> +{
>>>>>> + struct rpmsg_tty_port *cport;
>>>>>> +
>>>>>> + cport = kzalloc(sizeof(*cport), GFP_KERNEL);
>>>>>> + if (!cport)
>>>>>> + return ERR_PTR(-ENOMEM);
>>>>>> +
>>>>>> + mutex_lock(&idr_lock);
>>>>>> + cport->id = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
>>>>>> + mutex_unlock(&idr_lock);
>>>>>> +
>>>>>> + if (cport->id < 0) {
>>>>>> + kfree(cport);
>>>>>> + return ERR_PTR(-ENOSPC);
>>>>>> + }
>>>>>> +
>>>>>> + return cport;
>>>>>> +}
>>>>>> +
>>>>>> +static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport)
>>>>>> +{
>>>>>> + mutex_lock(&idr_lock);
>>>>>> + idr_remove(&tty_idr, cport->id);
>>>>>> + mutex_unlock(&idr_lock);
>>>>>> +
>>>>>> + kfree(cport);
>>>>>> +}
>>>>>> +
>>>>>> +static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
>>>>>> +{
>>>>>> + struct rpmsg_tty_port *cport;
>>>>>> + struct device *tty_dev;
>>>>>> + int ret;
>>>>>> +
>>>>>> + cport = rpmsg_tty_alloc_cport();
>>>>>> + if (IS_ERR(cport)) {
>>>>>> + dev_err(&rpdev->dev, "failed to alloc tty port\n");
>>>>>> + return PTR_ERR(cport);
>>>>>> + }
>>>>>> +
>>>>>> + tty_port_init(&cport->port);
>>>>>> + cport->port.low_latency = cport->port.flags | ASYNC_LOW_LATENCY;
>>>>>> +
>>>>>> + tty_dev = tty_port_register_device(&cport->port, rpmsg_tty_driver,
>>>>>> + cport->id, &rpdev->dev);
>>>>>> + if (IS_ERR(tty_dev)) {
>>>>>> + dev_err(&rpdev->dev, "failed to register tty port\n");
>>>>>> + ret = PTR_ERR(tty_dev);
>>>>>> + goto err_destroy;
>>>>>> + }
>>>>>> +
>>>>>> + spin_lock_init(&cport->rx_lock);
>>>>>> + cport->rpdev = rpdev;
>>>>>> +
>>>>>> + dev_set_drvdata(&rpdev->dev, cport);
>>>>>> +
>>>>>> + dev_info(&rpdev->dev, "new channel: 0x%x -> 0x%x : ttyRPMSG%d\n",
>>>>>> + rpdev->src, rpdev->dst, cport->id);
>>>>>> +
>>>>>> + return 0;
>>>>>> +
>>>>>> +err_destroy:
>>>>>> + tty_port_destroy(&cport->port);
>>>>>> + rpmsg_tty_release_cport(cport);
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static void rpmsg_tty_remove(struct rpmsg_device *rpdev)
>>>>>> +{
>>>>>> + struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
>>>>>> +
>>>>>> + dev_info(&rpdev->dev, "removing rpmsg tty device %d\n", cport->id);
>>>>>> +
>>>>>> + /* User hang up to release the tty */
>>>>>> + if (tty_port_initialized(&cport->port))
>>>>>> + tty_port_tty_hangup(&cport->port, false);
>>>>>> +
>>>>>> + tty_unregister_device(rpmsg_tty_driver, cport->id);
>>>>>> + tty_port_destroy(&cport->port);
>>>>>> +
>>>>>> + rpmsg_tty_release_cport(cport);
>>>>>> +}
>>>>>> +
>>>>>> +static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
>>>>>> + { .name = "rpmsg-tty-channel" },
>>>>>> + { },
>>>>>> +};
>>>>>> +MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_tty_id_table);
>>>>>> +
>>>>>> +static struct rpmsg_driver rpmsg_tty_rpmsg_drv = {
>>>>>> + .drv.name = KBUILD_MODNAME,
>>>>>> + .id_table = rpmsg_driver_tty_id_table,
>>>>>> + .probe = rpmsg_tty_probe,
>>>>>> + .callback = rpmsg_tty_cb,
>>>>>> + .remove = rpmsg_tty_remove,
>>>>>> +};
>>>>>> +
>>>>>> +static int __init rpmsg_tty_init(void)
>>>>>> +{
>>>>>> + int err;
>>>>>> +
>>>>>> + rpmsg_tty_driver = tty_alloc_driver(MAX_TTY_RPMSG, TTY_DRIVER_REAL_RAW |
>>>>>> + TTY_DRIVER_DYNAMIC_DEV);
>>>>>> + if (IS_ERR(rpmsg_tty_driver))
>>>>>> + return PTR_ERR(rpmsg_tty_driver);
>>>>>> +
>>>>>> + rpmsg_tty_driver->driver_name = "rpmsg_tty";
>>>>>> + rpmsg_tty_driver->name = "ttyRPMSG";
>>>>>> + rpmsg_tty_driver->major = 0;
>>>>>> + rpmsg_tty_driver->type = TTY_DRIVER_TYPE_CONSOLE;
>>>>>> + rpmsg_tty_driver->init_termios = tty_std_termios;
>>>>>> +
>>>>>> + tty_set_operations(rpmsg_tty_driver, &rpmsg_tty_ops);
>>>>>> +
>>>>>> + err = tty_register_driver(rpmsg_tty_driver);
>>>>>> + if (err < 0) {
>>>>>> + pr_err("Couldn't install rpmsg tty driver: err %d\n", err);
>>>>>> + goto error_put;
>>>>>> + }
>>>>>> +
>>>>>> + err = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
>>>>>> + if (err < 0) {
>>>>>> + pr_err("Couldn't register rpmsg tty driver: err %d\n", err);
>>>>>> + goto error_unregister;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +
>>>>>> +error_unregister:
>>>>>> + tty_unregister_driver(rpmsg_tty_driver);
>>>>>> +
>>>>>> +error_put:
>>>>>> + put_tty_driver(rpmsg_tty_driver);
>>>>>> +
>>>>>> + return err;
>>>>>> +}
>>>>>> +
>>>>>> +static void __exit rpmsg_tty_exit(void)
>>>>>> +{
>>>>>> + unregister_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
>>>>>> + tty_unregister_driver(rpmsg_tty_driver);
>>>>>> + put_tty_driver(rpmsg_tty_driver);
>>>>>> + idr_destroy(&tty_idr);
>>>>>> +}
>>>>>> +
>>>>>> +module_init(rpmsg_tty_init);
>>>>>> +module_exit(rpmsg_tty_exit);
>>>>>> +
>>>>>> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
>>>>>> +MODULE_AUTHOR("Fabien Dessenne <fabien.dessenne@st.com>");
>>>>>> +MODULE_DESCRIPTION("virtio remote processor messaging tty driver");
>>>>>> +MODULE_LICENSE("GPL v2");
>>>>>> --
>>>>>> 2.7.4
>>>>>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-04-05 16:08 ` Arnaud Pouliquen
@ 2019-04-06 7:56 ` xiang xiao
2019-04-08 12:05 ` Arnaud Pouliquen
0 siblings, 1 reply; 19+ messages in thread
From: xiang xiao @ 2019-04-06 7:56 UTC (permalink / raw)
To: Arnaud Pouliquen
Cc: Fabien Dessenne, Ohad Ben-Cohen, Bjorn Andersson,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-remoteproc,
Benjamin Gaignard
On Sat, Apr 6, 2019 at 12:08 AM Arnaud Pouliquen
<arnaud.pouliquen@st.com> wrote:
>
>
>
> On 4/5/19 4:03 PM, xiang xiao wrote:
> > On Fri, Apr 5, 2019 at 8:33 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
> >>
> >>
> >>
> >> On 4/5/19 12:12 PM, xiang xiao wrote:
> >>> On Fri, Apr 5, 2019 at 12:14 AM Arnaud Pouliquen
> >>> <arnaud.pouliquen@st.com> 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@st.com> 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.
> 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.
>
>
> > 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
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).
> 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.
> 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.
>
> >
> > 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.
>
> >
> >>>
> >>>> using some wrapper functions available here:
> >>>> https://github.com/STMicroelectronics/STM32CubeMP1/tree/master/Middlewares/Third_Party/OpenAMP
> >>>>
> >>>>
> >>>> Best Regards
> >>>> Arnaud
> >>>>
> >>>>>
> >>>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> >>>>>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> >>>>>> ---
> >>>>>> drivers/tty/Kconfig | 9 ++
> >>>>>> drivers/tty/Makefile | 1 +
> >>>>>> drivers/tty/rpmsg_tty.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>> 3 files changed, 319 insertions(+)
> >>>>>> create mode 100644 drivers/tty/rpmsg_tty.c
> >>>>>>
> >>>>>> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> >>>>>> index 0840d27..42696e6 100644
> >>>>>> --- a/drivers/tty/Kconfig
> >>>>>> +++ b/drivers/tty/Kconfig
> >>>>>> @@ -441,4 +441,13 @@ config VCC
> >>>>>> depends on SUN_LDOMS
> >>>>>> 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.
> >>>>>> endif # TTY
> >>>>>> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> >>>>>> index c72cafd..90a98a2 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
> >>>>>> new file mode 100644
> >>>>>> index 0000000..9c2a629
> >>>>>> --- /dev/null
> >>>>>> +++ b/drivers/tty/rpmsg_tty.c
> >>>>>> @@ -0,0 +1,309 @@
> >>>>>> +// SPDX-License-Identifier: GPL-2.0
> >>>>>> +/*
> >>>>>> + * Copyright (C) STMicroelectronics 2019 - All Rights Reserved
> >>>>>> + * Authors: Arnaud Pouliquen <arnaud.pouliquen@st.com> for STMicroelectronics.
> >>>>>> + * Fabien Dessenne <fabien.dessenne@st.com> for STMicroelectronics.
> >>>>>> + * Derived from the imx-rpmsg and omap-rpmsg implementations.
> >>>>>> + */
> >>>>>> +
> >>>>>> +#include <linux/module.h>
> >>>>>> +#include <linux/rpmsg.h>
> >>>>>> +#include <linux/slab.h>
> >>>>>> +#include <linux/tty.h>
> >>>>>> +#include <linux/tty_flip.h>
> >>>>>> +
> >>>>>> +#define MAX_TTY_RPMSG 32
> >>>>>> +
> >>>>>> +static DEFINE_IDR(tty_idr); /* tty instance id */
> >>>>>> +static DEFINE_MUTEX(idr_lock); /* protects tty_idr */
> >>>>>> +
> >>>>>> +static struct tty_driver *rpmsg_tty_driver;
> >>>>>> +
> >>>>>> +struct rpmsg_tty_port {
> >>>>>> + struct tty_port port; /* TTY port data */
> >>>>>> + unsigned int id; /* TTY rpmsg index */
> >>>>>> + spinlock_t rx_lock; /* message reception lock */
> >>>>>> + struct rpmsg_device *rpdev; /* rpmsg device */
> >>>>>> +};
> >>>>>> +
> >>>>>> +static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len,
> >>>>>> + void *priv, u32 src)
> >>>>>> +{
> >>>>>> + int space;
> >>>>>> + unsigned char *cbuf;
> >>>>>> + struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
> >>>>>> +
> >>>>>> + dev_dbg(&rpdev->dev, "msg(<- src 0x%x) len %d\n", src, len);
> >>>>>> +
> >>>>>> + print_hex_dump_debug(__func__, DUMP_PREFIX_NONE, 16, 1, data, len,
> >>>>>> + true);
> >>>>>> +
> >>>>>> + /* Flush the recv-ed none-zero data to tty node */
> >>>>>> + if (!len)
> >>>>>> + return -EINVAL;
> >>>>>> +
> >>>>>> + spin_lock(&cport->rx_lock);
> >>>>>> + space = tty_prepare_flip_string(&cport->port, &cbuf, len);
> >>>>>> + if (space <= 0) {
> >>>>>> + dev_err(&rpdev->dev, "No memory for tty_prepare_flip_string\n");
> >>>>>> + spin_unlock(&cport->rx_lock);
> >>>>>> + return -ENOMEM;
> >>>>>> + }
> >>>>>> +
> >>>>>> + if (space != len)
> >>>>>> + dev_warn(&rpdev->dev, "Trunc buffer from %d to %d\n",
> >>>>>> + len, space);
> >>>>>> +
> >>>>>> + memcpy(cbuf, data, space);
> >>>>>> + tty_flip_buffer_push(&cport->port);
> >>>>>> + spin_unlock(&cport->rx_lock);
> >>>>>> +
> >>>>>> + return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int rpmsg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
> >>>>>> +{
> >>>>>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> >>>>>> +
> >>>>>> + if (!cport) {
> >>>>>> + dev_err(tty->dev, "cannot get cport\n");
> >>>>>> + return -ENODEV;
> >>>>>> + }
> >>>>>> +
> >>>>>> + return tty_port_install(&cport->port, driver, tty);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int rpmsg_tty_open(struct tty_struct *tty, struct file *filp)
> >>>>>> +{
> >>>>>> + return tty_port_open(tty->port, tty, filp);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void rpmsg_tty_close(struct tty_struct *tty, struct file *filp)
> >>>>>> +{
> >>>>>> + return tty_port_close(tty->port, tty, filp);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int rpmsg_tty_write(struct tty_struct *tty, const unsigned char *buf,
> >>>>>> + int total)
> >>>>>> +{
> >>>>>> + int count, ret = 0;
> >>>>>> + const unsigned char *tbuf;
> >>>>>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> >>>>>> + struct rpmsg_device *rpdev;
> >>>>>> + int msg_size;
> >>>>>> +
> >>>>>> + if (!cport) {
> >>>>>> + dev_err(tty->dev, "cannot get cport\n");
> >>>>>> + return -ENODEV;
> >>>>>> + }
> >>>>>> +
> >>>>>> + rpdev = cport->rpdev;
> >>>>>> +
> >>>>>> + dev_dbg(&rpdev->dev, "%s: send message from tty->index = %d\n",
> >>>>>> + __func__, tty->index);
> >>>>>> +
> >>>>>> + if (!buf) {
> >>>>>> + dev_err(&rpdev->dev, "buf shouldn't be null.\n");
> >>>>>> + return -ENOMEM;
> >>>>>> + }
> >>>>>> +
> >>>>>> + msg_size = rpmsg_get_buf_payload_size(rpdev->ept);
> >>>>>> + if (msg_size < 0)
> >>>>>> + return msg_size;
> >>>>>> +
> >>>>>> + count = total;
> >>>>>> + tbuf = buf;
> >>>>>> + do {
> >>>>>> + /* send a message to our remote processor */
> >>>>>> + ret = rpmsg_send(rpdev->ept, (void *)tbuf,
> >>>>>> + min(count, msg_size));
> >>>>>> + if (ret) {
> >>>>>> + dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
> >>>>>> + return ret;
> >>>>>> + }
> >>>>>> +
> >>>>>> + if (count > msg_size) {
> >>>>>> + count -= msg_size;
> >>>>>> + tbuf += msg_size;
> >>>>>> + } else {
> >>>>>> + count = 0;
> >>>>>> + }
> >>>>>> + } while (count > 0);
> >>>>>> +
> >>>>>
> >>>>> We need the flow control(or handshake) here, otherwise it's very easy
> >>>>> to lose the data if kernel keep send the data as fast as possible.
> >>>>>
> >>>>>> + return total;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int rpmsg_tty_write_room(struct tty_struct *tty)
> >>>>>> +{
> >>>>>> + struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> >>>>>> +
> >>>>>> + if (!cport) {
> >>>>>> + dev_err(tty->dev, "cannot get cport\n");
> >>>>>> + return -ENODEV;
> >>>>>> + }
> >>>>>> +
> >>>>>> + /* report the space in the rpmsg buffer */
> >>>>>> + return rpmsg_get_buf_payload_size(cport->rpdev->ept);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static const struct tty_operations rpmsg_tty_ops = {
> >>>>>> + .install = rpmsg_tty_install,
> >>>>>> + .open = rpmsg_tty_open,
> >>>>>> + .close = rpmsg_tty_close,
> >>>>>> + .write = rpmsg_tty_write,
> >>>>>> + .write_room = rpmsg_tty_write_room,
> >>>>>> +};
> >>>>>> +
> >>>>>> +struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
> >>>>>> +{
> >>>>>> + struct rpmsg_tty_port *cport;
> >>>>>> +
> >>>>>> + cport = kzalloc(sizeof(*cport), GFP_KERNEL);
> >>>>>> + if (!cport)
> >>>>>> + return ERR_PTR(-ENOMEM);
> >>>>>> +
> >>>>>> + mutex_lock(&idr_lock);
> >>>>>> + cport->id = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
> >>>>>> + mutex_unlock(&idr_lock);
> >>>>>> +
> >>>>>> + if (cport->id < 0) {
> >>>>>> + kfree(cport);
> >>>>>> + return ERR_PTR(-ENOSPC);
> >>>>>> + }
> >>>>>> +
> >>>>>> + return cport;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport)
> >>>>>> +{
> >>>>>> + mutex_lock(&idr_lock);
> >>>>>> + idr_remove(&tty_idr, cport->id);
> >>>>>> + mutex_unlock(&idr_lock);
> >>>>>> +
> >>>>>> + kfree(cport);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
> >>>>>> +{
> >>>>>> + struct rpmsg_tty_port *cport;
> >>>>>> + struct device *tty_dev;
> >>>>>> + int ret;
> >>>>>> +
> >>>>>> + cport = rpmsg_tty_alloc_cport();
> >>>>>> + if (IS_ERR(cport)) {
> >>>>>> + dev_err(&rpdev->dev, "failed to alloc tty port\n");
> >>>>>> + return PTR_ERR(cport);
> >>>>>> + }
> >>>>>> +
> >>>>>> + tty_port_init(&cport->port);
> >>>>>> + cport->port.low_latency = cport->port.flags | ASYNC_LOW_LATENCY;
> >>>>>> +
> >>>>>> + tty_dev = tty_port_register_device(&cport->port, rpmsg_tty_driver,
> >>>>>> + cport->id, &rpdev->dev);
> >>>>>> + if (IS_ERR(tty_dev)) {
> >>>>>> + dev_err(&rpdev->dev, "failed to register tty port\n");
> >>>>>> + ret = PTR_ERR(tty_dev);
> >>>>>> + goto err_destroy;
> >>>>>> + }
> >>>>>> +
> >>>>>> + spin_lock_init(&cport->rx_lock);
> >>>>>> + cport->rpdev = rpdev;
> >>>>>> +
> >>>>>> + dev_set_drvdata(&rpdev->dev, cport);
> >>>>>> +
> >>>>>> + dev_info(&rpdev->dev, "new channel: 0x%x -> 0x%x : ttyRPMSG%d\n",
> >>>>>> + rpdev->src, rpdev->dst, cport->id);
> >>>>>> +
> >>>>>> + return 0;
> >>>>>> +
> >>>>>> +err_destroy:
> >>>>>> + tty_port_destroy(&cport->port);
> >>>>>> + rpmsg_tty_release_cport(cport);
> >>>>>> + return ret;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void rpmsg_tty_remove(struct rpmsg_device *rpdev)
> >>>>>> +{
> >>>>>> + struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
> >>>>>> +
> >>>>>> + dev_info(&rpdev->dev, "removing rpmsg tty device %d\n", cport->id);
> >>>>>> +
> >>>>>> + /* User hang up to release the tty */
> >>>>>> + if (tty_port_initialized(&cport->port))
> >>>>>> + tty_port_tty_hangup(&cport->port, false);
> >>>>>> +
> >>>>>> + tty_unregister_device(rpmsg_tty_driver, cport->id);
> >>>>>> + tty_port_destroy(&cport->port);
> >>>>>> +
> >>>>>> + rpmsg_tty_release_cport(cport);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
> >>>>>> + { .name = "rpmsg-tty-channel" },
> >>>>>> + { },
> >>>>>> +};
> >>>>>> +MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_tty_id_table);
> >>>>>> +
> >>>>>> +static struct rpmsg_driver rpmsg_tty_rpmsg_drv = {
> >>>>>> + .drv.name = KBUILD_MODNAME,
> >>>>>> + .id_table = rpmsg_driver_tty_id_table,
> >>>>>> + .probe = rpmsg_tty_probe,
> >>>>>> + .callback = rpmsg_tty_cb,
> >>>>>> + .remove = rpmsg_tty_remove,
> >>>>>> +};
> >>>>>> +
> >>>>>> +static int __init rpmsg_tty_init(void)
> >>>>>> +{
> >>>>>> + int err;
> >>>>>> +
> >>>>>> + rpmsg_tty_driver = tty_alloc_driver(MAX_TTY_RPMSG, TTY_DRIVER_REAL_RAW |
> >>>>>> + TTY_DRIVER_DYNAMIC_DEV);
> >>>>>> + if (IS_ERR(rpmsg_tty_driver))
> >>>>>> + return PTR_ERR(rpmsg_tty_driver);
> >>>>>> +
> >>>>>> + rpmsg_tty_driver->driver_name = "rpmsg_tty";
> >>>>>> + rpmsg_tty_driver->name = "ttyRPMSG";
> >>>>>> + rpmsg_tty_driver->major = 0;
> >>>>>> + rpmsg_tty_driver->type = TTY_DRIVER_TYPE_CONSOLE;
> >>>>>> + rpmsg_tty_driver->init_termios = tty_std_termios;
> >>>>>> +
> >>>>>> + tty_set_operations(rpmsg_tty_driver, &rpmsg_tty_ops);
> >>>>>> +
> >>>>>> + err = tty_register_driver(rpmsg_tty_driver);
> >>>>>> + if (err < 0) {
> >>>>>> + pr_err("Couldn't install rpmsg tty driver: err %d\n", err);
> >>>>>> + goto error_put;
> >>>>>> + }
> >>>>>> +
> >>>>>> + err = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
> >>>>>> + if (err < 0) {
> >>>>>> + pr_err("Couldn't register rpmsg tty driver: err %d\n", err);
> >>>>>> + goto error_unregister;
> >>>>>> + }
> >>>>>> +
> >>>>>> + return 0;
> >>>>>> +
> >>>>>> +error_unregister:
> >>>>>> + tty_unregister_driver(rpmsg_tty_driver);
> >>>>>> +
> >>>>>> +error_put:
> >>>>>> + put_tty_driver(rpmsg_tty_driver);
> >>>>>> +
> >>>>>> + return err;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void __exit rpmsg_tty_exit(void)
> >>>>>> +{
> >>>>>> + unregister_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
> >>>>>> + tty_unregister_driver(rpmsg_tty_driver);
> >>>>>> + put_tty_driver(rpmsg_tty_driver);
> >>>>>> + idr_destroy(&tty_idr);
> >>>>>> +}
> >>>>>> +
> >>>>>> +module_init(rpmsg_tty_init);
> >>>>>> +module_exit(rpmsg_tty_exit);
> >>>>>> +
> >>>>>> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
> >>>>>> +MODULE_AUTHOR("Fabien Dessenne <fabien.dessenne@st.com>");
> >>>>>> +MODULE_DESCRIPTION("virtio remote processor messaging tty driver");
> >>>>>> +MODULE_LICENSE("GPL v2");
> >>>>>> --
> >>>>>> 2.7.4
> >>>>>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-04-06 7:56 ` xiang xiao
@ 2019-04-08 12:05 ` Arnaud Pouliquen
2019-04-08 13:29 ` xiang xiao
0 siblings, 1 reply; 19+ messages in thread
From: Arnaud Pouliquen @ 2019-04-08 12:05 UTC (permalink / raw)
To: xiang xiao
Cc: Fabien Dessenne, Ohad Ben-Cohen, Bjorn Andersson,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-remoteproc,
Benjamin Gaignard
On 4/6/19 9:56 AM, xiang xiao wrote:
> On Sat, Apr 6, 2019 at 12:08 AM Arnaud Pouliquen
> <arnaud.pouliquen@st.com> wrote:
>>
>>
>>
>> On 4/5/19 4:03 PM, xiang xiao wrote:
>>> On Fri, Apr 5, 2019 at 8:33 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>>>>
>>>>
>>>>
>>>> On 4/5/19 12:12 PM, xiang xiao wrote:
>>>>> On Fri, Apr 5, 2019 at 12:14 AM Arnaud Pouliquen
>>>>> <arnaud.pouliquen@st.com> 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@st.com> 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.
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.
>
>> 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.
>
>>
>>
>>> 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.
>
>> 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.
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
...
>
>>
>>>
>>> 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.
>>
>>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-04-08 12:05 ` Arnaud Pouliquen
@ 2019-04-08 13:29 ` xiang xiao
2019-04-09 7:28 ` Arnaud Pouliquen
0 siblings, 1 reply; 19+ messages in thread
From: xiang xiao @ 2019-04-08 13:29 UTC (permalink / raw)
To: Arnaud Pouliquen
Cc: Fabien Dessenne, Ohad Ben-Cohen, Bjorn Andersson,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-remoteproc,
Benjamin Gaignard
On Mon, Apr 8, 2019 at 8:05 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>
>
>
> On 4/6/19 9:56 AM, xiang xiao wrote:
> > On Sat, Apr 6, 2019 at 12:08 AM Arnaud Pouliquen
> > <arnaud.pouliquen@st.com> wrote:
> >>
> >>
> >>
> >> On 4/5/19 4:03 PM, xiang xiao wrote:
> >>> On Fri, Apr 5, 2019 at 8:33 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 4/5/19 12:12 PM, xiang xiao wrote:
> >>>>> On Fri, Apr 5, 2019 at 12:14 AM Arnaud Pouliquen
> >>>>> <arnaud.pouliquen@st.com> 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@st.com> 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.
> >>
> >>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-04-08 13:29 ` xiang xiao
@ 2019-04-09 7:28 ` Arnaud Pouliquen
2019-04-09 10:14 ` xiang xiao
0 siblings, 1 reply; 19+ messages in thread
From: Arnaud Pouliquen @ 2019-04-09 7:28 UTC (permalink / raw)
To: xiang xiao
Cc: Fabien Dessenne, Ohad Ben-Cohen, Bjorn Andersson,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-remoteproc,
Benjamin Gaignard
On 4/8/19 3:29 PM, xiang xiao wrote:
> On Mon, Apr 8, 2019 at 8:05 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>>
>>
>>
>> On 4/6/19 9:56 AM, xiang xiao wrote:
>>> On Sat, Apr 6, 2019 at 12:08 AM Arnaud Pouliquen
>>> <arnaud.pouliquen@st.com> wrote:
>>>>
>>>>
>>>>
>>>> On 4/5/19 4:03 PM, xiang xiao wrote:
>>>>> On Fri, Apr 5, 2019 at 8:33 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 4/5/19 12:12 PM, xiang xiao wrote:
>>>>>>> On Fri, Apr 5, 2019 at 12:14 AM Arnaud Pouliquen
>>>>>>> <arnaud.pouliquen@st.com> 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@st.com> 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.
Today this not the way rpmsg implements the service but declares it on
registration. This is an evolution of the rpmsg, so better to propose it
in a separate thread.
>
>> 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?In your uart_rpmsg/c.
the rpmsg service is "rpmsg-tty" this is a "standard" service. But you
define a "rpmsg-ttyxxxx" service because you want to expose a service on
top of the tty service, not the tty service itself. In this way you are
not able to list this service in rpmsg_device_id because not standard
static service, you have to implement the match. This look like you
adapt rpmsg protocol to match with your platform implementation.
>
>>>
>>>> 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.
when i have a look in /dev/tty, a number is generaly used to instantiate
the same device type. For instance if you have several tty over USB, you
have several instantiation of the ttyACM, nothing linked to the service
on top of the link.
Here from my point of view it is the same.
> 2.rpmsg protocol support name and port mapping natively, why not use it?
Precisely we want to use native implementation of the protocol, not to
extend it with the match function that introduces a meta service notion.
I'm not sure that we can find a compromise on this point. So I would
like to propose you to do this in 2 steps:
step 1: we start on basic RPMsg service, (with ept addr as port ID, if
you are interesting in).
step 2: you send patch on top to propose rpmsg match function, with tty
naming based on feature name (with support of the legacy).
>>>
>>>>
>>>>
>>>>> 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.
But i still not understand while we should wait an answer on a message.
The ack should be client dependent, not part of the protocol.
Furthemore a issue of this is that the line discipline allows to echo
every chars received on tty dev. This would generate an infinite loop as
the remote also echo it.
>
>>
>>>
>>>> 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?
RTS management could help for this.
>
>> 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.
You are right the control flow is missing in our implementation.
This is also an elegant way to enable the communication in both direction.
So the wakeup in your implementation is used for this, right?
Do you test the dtr_rts ops on your side?
tty_port_operations ops should also be added...
>
>>
>>
>>>
>>>>
>>>>>
>>>>> 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.
>>>>
>>>>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-04-09 7:28 ` Arnaud Pouliquen
@ 2019-04-09 10:14 ` xiang xiao
2019-04-12 16:00 ` Arnaud Pouliquen
0 siblings, 1 reply; 19+ messages in thread
From: xiang xiao @ 2019-04-09 10:14 UTC (permalink / raw)
To: Arnaud Pouliquen
Cc: Fabien Dessenne, Ohad Ben-Cohen, Bjorn Andersson,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-remoteproc,
Benjamin Gaignard
On Tue, Apr 9, 2019 at 3:28 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>
>
>
> On 4/8/19 3:29 PM, xiang xiao wrote:
> > On Mon, Apr 8, 2019 at 8:05 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
> >>
> >>
> >>
> >> On 4/6/19 9:56 AM, xiang xiao wrote:
> >>> On Sat, Apr 6, 2019 at 12:08 AM Arnaud Pouliquen
> >>> <arnaud.pouliquen@st.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 4/5/19 4:03 PM, xiang xiao wrote:
> >>>>> On Fri, Apr 5, 2019 at 8:33 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 4/5/19 12:12 PM, xiang xiao wrote:
> >>>>>>> On Fri, Apr 5, 2019 at 12:14 AM Arnaud Pouliquen
> >>>>>>> <arnaud.pouliquen@st.com> 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@st.com> 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.
> Today this not the way rpmsg implements the service but declares it on
> registration. This is an evolution of the rpmsg, so better to propose it
> in a separate thread.
>
Here is the patch I post before:
https://patchwork.kernel.org/patch/10791741/
> >
> >> 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?In your uart_rpmsg/c.
> the rpmsg service is "rpmsg-tty" this is a "standard" service. But you
> define a "rpmsg-ttyxxxx" service because you want to expose a service on
> top of the tty service, not the tty service itself. In this way you are
> not able to list this service in rpmsg_device_id because not standard
> static service, you have to implement the match. This look like you
> adapt rpmsg protocol to match with your platform implementation.
>
> >
> >>>
> >>>> 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.
> when i have a look in /dev/tty, a number is generaly used to instantiate
> the same device type. For instance if you have several tty over USB, you
> have several instantiation of the ttyACM, nothing linked to the service
> on top of the link.
> Here from my point of view it is the same.
>
> > 2.rpmsg protocol support name and port mapping natively, why not use it?
> Precisely we want to use native implementation of the protocol, not to
> extend it with the match function that introduces a meta service notion.
>
> I'm not sure that we can find a compromise on this point. So I would
> like to propose you to do this in 2 steps:
> step 1: we start on basic RPMsg service, (with ept addr as port ID, if
> you are interesting in).
> step 2: you send patch on top to propose rpmsg match function, with tty
> naming based on feature name (with support of the legacy).
>
It is fine to put the naming tty to another patch.
> >>>
> >>>>
> >>>>
> >>>>> 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.
> But i still not understand while we should wait an answer on a message.
With the slide window algo:
1.Exchange the buffer size at the beginning
2.Any side send data and decrease the buffer size
3.Implement write_room callback to return the left buffer size
4.tty framework stop to call write callack if write_room return 0
Since rpmsg transport is reliable, the receiver don't need send
acknowledge and then the sender don't need wait the response.
But the receiver need to send the message to report the new slide
window size after anybody read some data from buffer.
> The ack should be client dependent, not part of the protocol.
> Furthemore a issue of this is that the line discipline allows to echo
> every chars received on tty dev. This would generate an infinite loop as
> the remote also echo it.
For the loopback test, we should disable line discipline echo.
>
> >
> >>
> >>>
> >>>> 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?
> RTS management could help for this.
>
> >
> >> 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.
> You are right the control flow is missing in our implementation.
> This is also an elegant way to enable the communication in both direction.
> So the wakeup in your implementation is used for this, right?
Yes, wakeup mean the peer has more free buffer again, it is very
simple but not effiecent as the slide window algo.
> Do you test the dtr_rts ops on your side?
> tty_port_operations ops should also be added...
>
dtr_rts just add the complexity, but don't have any benifit, because:
1.The flow control support isn't very expense(e.g. no RTC/CTS pin)
2.All existed virtual channel is always reliable(fifo, pipe and domain socket)
I prefer to make rpmsg tty always reliable and disable userspace change it.
> >
> >>
> >>
> >>>
> >>>>
> >>>>>
> >>>>> 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.
> >>>>
> >>>>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-04-09 10:14 ` xiang xiao
@ 2019-04-12 16:00 ` Arnaud Pouliquen
2019-04-15 13:14 ` Enrico Weigelt, metux IT consult
0 siblings, 1 reply; 19+ messages in thread
From: Arnaud Pouliquen @ 2019-04-12 16:00 UTC (permalink / raw)
To: xiang xiao
Cc: Fabien Dessenne, Ohad Ben-Cohen, Bjorn Andersson,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-remoteproc,
Benjamin Gaignard
On 4/9/19 12:14 PM, xiang xiao wrote:
> On Tue, Apr 9, 2019 at 3:28 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>>
>>
>>
>> On 4/8/19 3:29 PM, xiang xiao wrote:
>>> On Mon, Apr 8, 2019 at 8:05 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>>>>
>>>>
>>>>
>>>> On 4/6/19 9:56 AM, xiang xiao wrote:
>>>>> On Sat, Apr 6, 2019 at 12:08 AM Arnaud Pouliquen
>>>>> <arnaud.pouliquen@st.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 4/5/19 4:03 PM, xiang xiao wrote:
>>>>>>> On Fri, Apr 5, 2019 at 8:33 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4/5/19 12:12 PM, xiang xiao wrote:
>>>>>>>>> On Fri, Apr 5, 2019 at 12:14 AM Arnaud Pouliquen
>>>>>>>>> <arnaud.pouliquen@st.com> 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@st.com> 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.
>> Today this not the way rpmsg implements the service but declares it on
>> registration. This is an evolution of the rpmsg, so better to propose it
>> in a separate thread.
>>
>
> Here is the patch I post before:
> https://patchwork.kernel.org/patch/10791741/
>
>>>
>>>> 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?In your uart_rpmsg/c.
>> the rpmsg service is "rpmsg-tty" this is a "standard" service. But you
>> define a "rpmsg-ttyxxxx" service because you want to expose a service on
>> top of the tty service, not the tty service itself. In this way you are
>> not able to list this service in rpmsg_device_id because not standard
>> static service, you have to implement the match. This look like you
>> adapt rpmsg protocol to match with your platform implementation.
>>
>>>
>>>>>
>>>>>> 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.
>> when i have a look in /dev/tty, a number is generaly used to instantiate
>> the same device type. For instance if you have several tty over USB, you
>> have several instantiation of the ttyACM, nothing linked to the service
>> on top of the link.
>> Here from my point of view it is the same.
>>
>>> 2.rpmsg protocol support name and port mapping natively, why not use it?
>> Precisely we want to use native implementation of the protocol, not to
>> extend it with the match function that introduces a meta service notion.
>>
>> I'm not sure that we can find a compromise on this point. So I would
>> like to propose you to do this in 2 steps:
>> step 1: we start on basic RPMsg service, (with ept addr as port ID, if
>> you are interesting in).
>> step 2: you send patch on top to propose rpmsg match function, with tty
>> naming based on feature name (with support of the legacy).
>>
>
> It is fine to put the naming tty to another patch.
For the first step: I tested the use of ept dest address as index, not
possible as it is used by core part as table index. I have to keep basic
indexation.
So this will give you argument for your add-on patch.
>
>>>>>
>>>>>>
>>>>>>
>>>>>>> 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.
>> But i still not understand while we should wait an answer on a message.
>
> With the slide window algo:
> 1.Exchange the buffer size at the beginning
> 2.Any side send data and decrease the buffer size
> 3.Implement write_room callback to return the left buffer size
> 4.tty framework stop to call write callack if write_room return 0
> Since rpmsg transport is reliable, the receiver don't need send
> acknowledge and then the sender don't need wait the response.
> But the receiver need to send the message to report the new slide
> window size after anybody read some data from buffer.
Seems very complex for a tty purpose.
If room is set to RPMSG buffer size the bottle-neck is the
RPMsg buffers availability.
This should be detected by returning 0 on write if no buffer available,
using rpmsg_trysend.
Now i can see 2 use-cases that could need flow control.
1) receiver want to stop the transfer in reception:
=> similar to RTS/CTS
2) need flow control on RPMsg to share the buffer between different
services (not only tty).
=> In this second case this should be manage in RPMsg. This need has
already been identified during discussion in community. Could be managed
based on a max bandwith request ( size or number of RPMsg buffer)
controlled by the RPMsg core...
>
>> The ack should be client dependent, not part of the protocol.
>> Furthemore a issue of this is that the line discipline allows to echo
>> every chars received on tty dev. This would generate an infinite loop as
>> the remote also echo it.
>
> For the loopback test, we should disable line discipline echo.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-04-12 16:00 ` Arnaud Pouliquen
@ 2019-04-15 13:14 ` Enrico Weigelt, metux IT consult
2019-04-15 13:50 ` xiang xiao
0 siblings, 1 reply; 19+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2019-04-15 13:14 UTC (permalink / raw)
To: Arnaud Pouliquen, xiang xiao
Cc: Fabien Dessenne, Ohad Ben-Cohen, Bjorn Andersson,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-remoteproc,
Benjamin Gaignard
On 12.04.19 18:00, Arnaud Pouliquen wrote:
Hi folks,
<snip>
Haven't followed the whole thread, but I've got the impression that the
device is emulating an uart - if that's the case wouldn't it be better
to implement a serial driver, instead of tty directly (which IMHO should
make it also usable for serbus-based devices) ?
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] tty: add rpmsg driver
2019-04-15 13:14 ` Enrico Weigelt, metux IT consult
@ 2019-04-15 13:50 ` xiang xiao
0 siblings, 0 replies; 19+ messages in thread
From: xiang xiao @ 2019-04-15 13:50 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult
Cc: Arnaud Pouliquen, Fabien Dessenne, Ohad Ben-Cohen,
Bjorn Andersson, Greg Kroah-Hartman, Jiri Slaby, linux-kernel,
linux-remoteproc, Benjamin Gaignard
On Mon, Apr 15, 2019 at 9:14 PM Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
>
> On 12.04.19 18:00, Arnaud Pouliquen wrote:
>
> Hi folks,
>
> <snip>
>
> Haven't followed the whole thread, but I've got the impression that the
> device is emulating an uart - if that's the case wouldn't it be better
> to implement a serial driver, instead of tty directly (which IMHO should
> make it also usable for serbus-based devices) ?
It's more natural to implement rpmsg uart on top of tty core instead
of serial layer, since:
1.Serial layer model the hardware which can just send the data one by one
2.But rpmsg could send a buffer(max 512B) in one packet
On the other hand, it's very easy to support serbus by replace
tty_port_register_device_attr to tty_port_register_device_attr_serdev.
>
>
> --mtx
>
> --
> Enrico Weigelt, metux IT consult
> Free software and Linux embedded engineering
> info@metux.net -- +49-151-27565287
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2019-04-15 13:51 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-21 15:47 [PATCH 0/2] TTY: add rpmsg tty driver Fabien Dessenne
2019-03-21 15:47 ` [PATCH 1/2] rpmsg: core: add possibility to get message payload length Fabien Dessenne
2019-03-21 15:47 ` [PATCH 2/2] tty: add rpmsg driver Fabien Dessenne
2019-04-03 8:44 ` Johan Hovold
2019-04-05 12:50 ` Fabien DESSENNE
2019-04-03 12:47 ` xiang xiao
2019-04-04 16:14 ` Arnaud Pouliquen
2019-04-05 10:12 ` xiang xiao
2019-04-05 12:33 ` Arnaud Pouliquen
2019-04-05 14:03 ` xiang xiao
2019-04-05 16:08 ` Arnaud Pouliquen
2019-04-06 7:56 ` xiang xiao
2019-04-08 12:05 ` Arnaud Pouliquen
2019-04-08 13:29 ` xiang xiao
2019-04-09 7:28 ` Arnaud Pouliquen
2019-04-09 10:14 ` xiang xiao
2019-04-12 16:00 ` Arnaud Pouliquen
2019-04-15 13:14 ` Enrico Weigelt, metux IT consult
2019-04-15 13:50 ` xiang xiao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).