Add new driver to allow tty over i3c master.
Signed-off-by: Frank Li <[email protected]>
---
Notes:
This patch depend on
https://lore.kernel.org/imx/[email protected]/T/#t
drivers/tty/Kconfig | 8 +
drivers/tty/Makefile | 1 +
drivers/tty/i3c_tty.c | 466 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 475 insertions(+)
create mode 100644 drivers/tty/i3c_tty.c
diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index 5646dc6242cd9..6d91fe6a211a1 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -412,6 +412,14 @@ config RPMSG_TTY
To compile this driver as a module, choose M here: the module will be
called rpmsg_tty.
+config I3C_TTY
+ tristate "tty over i3c"
+ depends on I3C
+ help
+ Select this options if you'd like use tty over I3C master controller
+
+ If unsure, say N
+
endif # TTY
source "drivers/tty/serdev/Kconfig"
diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
index 07aca5184a55d..f329f9c7d308a 100644
--- a/drivers/tty/Makefile
+++ b/drivers/tty/Makefile
@@ -27,5 +27,6 @@ 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-$(CONFIG_I3C_TTY) += i3c_tty.o
obj-y += ipwireless/
diff --git a/drivers/tty/i3c_tty.c b/drivers/tty/i3c_tty.c
new file mode 100644
index 0000000000000..fe45bf94a8cf2
--- /dev/null
+++ b/drivers/tty/i3c_tty.c
@@ -0,0 +1,466 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2023 NXP.
+ *
+ * Author: Frank Li <[email protected]>
+ */
+
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/i3c/device.h>
+#include <linux/i3c/master.h>
+#include <linux/slab.h>
+#include <linux/console.h>
+#include <linux/serial_core.h>
+#include <linux/interrupt.h>
+#include <linux/workqueue.h>
+#include <linux/tty_flip.h>
+
+static DEFINE_IDR(i3c_tty_minors);
+static DEFINE_MUTEX(i3c_tty_minors_lock);
+
+static struct tty_driver *i3c_tty_driver;
+
+#define I3C_TTY_MINORS 256
+#define I3C_TTY_TRANS_SIZE 16
+#define I3C_TTY_RX_STOP BIT(0)
+#define I3C_TTY_RETRY 20
+#define I3C_TTY_YIELD_US 100
+
+struct ttyi3c_port {
+ struct tty_port port;
+ int minor;
+ unsigned int txfifo_size;
+ unsigned int rxfifo_size;
+ struct circ_buf xmit;
+ spinlock_t xlock; /* protect xmit */
+ void *buffer;
+ struct i3c_device *i3cdev;
+ struct work_struct txwork;
+ struct work_struct rxwork;
+ struct completion txcomplete;
+ struct workqueue_struct *workqueue;
+ atomic_t status;
+};
+
+static const struct i3c_device_id i3c_ids[] = {
+ I3C_DEVICE(0x011B, 0x1000, NULL),
+ { /* sentinel */ },
+};
+
+static int i3c_port_activate(struct tty_port *port, struct tty_struct *tty)
+{
+ struct ttyi3c_port *sport = container_of(port, struct ttyi3c_port, port);
+
+ atomic_set(&sport->status, 0);
+
+ return i3c_device_enable_ibi(sport->i3cdev);
+}
+
+static void i3c_port_shutdown(struct tty_port *port)
+{
+ struct ttyi3c_port *sport =
+ container_of(port, struct ttyi3c_port, port);
+
+ i3c_device_disable_ibi(sport->i3cdev);
+}
+
+static void i3c_port_destruct(struct tty_port *port)
+{
+ struct ttyi3c_port *sport =
+ container_of(port, struct ttyi3c_port, port);
+
+ mutex_lock(&i3c_tty_minors_lock);
+ idr_remove(&i3c_tty_minors, sport->minor);
+ mutex_unlock(&i3c_tty_minors_lock);
+}
+
+static const struct tty_port_operations i3c_port_ops = {
+ .shutdown = i3c_port_shutdown,
+ .activate = i3c_port_activate,
+ .destruct = i3c_port_destruct,
+};
+
+static struct ttyi3c_port *i3c_get_by_minor(unsigned int minor)
+{
+ struct ttyi3c_port *sport;
+
+ mutex_lock(&i3c_tty_minors_lock);
+ sport = idr_find(&i3c_tty_minors, minor);
+ mutex_unlock(&i3c_tty_minors_lock);
+
+ return sport;
+}
+
+static int i3c_install(struct tty_driver *driver, struct tty_struct *tty)
+{
+ struct ttyi3c_port *sport;
+ int ret;
+
+ sport = i3c_get_by_minor(tty->index);
+ if (!sport)
+ return -ENODEV;
+
+ ret = tty_standard_install(driver, tty);
+ if (ret)
+ return ret;
+
+ tty->driver_data = sport;
+
+ return 0;
+}
+
+static ssize_t i3c_write(struct tty_struct *tty, const unsigned char *buf, size_t count)
+{
+ struct ttyi3c_port *sport = tty->driver_data;
+ struct circ_buf *circ = &sport->xmit;
+ unsigned long flags;
+ int c, ret = 0;
+
+ spin_lock_irqsave(&sport->xlock, flags);
+ while (1) {
+ c = CIRC_SPACE_TO_END(circ->head, circ->tail, UART_XMIT_SIZE);
+ if (count < c)
+ c = count;
+ if (c <= 0)
+ break;
+
+ memcpy(circ->buf + circ->head, buf, c);
+ circ->head = (circ->head + c) & (UART_XMIT_SIZE - 1);
+ buf += c;
+ count -= c;
+ ret += c;
+ }
+ spin_unlock_irqrestore(&sport->xlock, flags);
+
+ if (circ->head != circ->tail)
+ queue_work(sport->workqueue, &sport->txwork);
+
+ return ret;
+}
+
+static int i3c_put_char(struct tty_struct *tty, unsigned char ch)
+{
+ struct ttyi3c_port *sport = tty->driver_data;
+ struct circ_buf *circ = &sport->xmit;
+ unsigned long flags;
+ int ret = 0;
+
+ spin_lock_irqsave(&sport->xlock, flags);
+
+ if (sport && CIRC_SPACE(circ->head, circ->tail, UART_XMIT_SIZE) != 0) {
+ circ->buf[circ->head] = ch;
+ circ->head = (circ->head + 1) & (UART_XMIT_SIZE - 1);
+ ret = 1;
+ }
+
+ spin_unlock_irqrestore(&sport->xlock, flags);
+
+ return ret;
+}
+
+static void i3c_flush_chars(struct tty_struct *tty)
+{
+ struct ttyi3c_port *sport = tty->driver_data;
+
+ queue_work(sport->workqueue, &sport->txwork);
+}
+
+static unsigned int i3c_write_room(struct tty_struct *tty)
+{
+ struct ttyi3c_port *sport = tty->driver_data;
+ struct circ_buf *circ = &sport->xmit;
+ unsigned long flags;
+ int ret = 0;
+
+ spin_lock_irqsave(&sport->xlock, flags);
+ ret = CIRC_SPACE(circ->head, circ->tail, UART_XMIT_SIZE);
+ spin_unlock_irqrestore(&sport->xlock, flags);
+
+ return ret;
+}
+
+static void i3c_throttle(struct tty_struct *tty)
+{
+ struct ttyi3c_port *sport = tty->driver_data;
+
+ atomic_or(I3C_TTY_RX_STOP, &sport->status);
+}
+
+static void i3c_unthrottle(struct tty_struct *tty)
+{
+ struct ttyi3c_port *sport = tty->driver_data;
+
+ atomic_andnot(I3C_TTY_RX_STOP, &sport->status);
+
+ queue_work(sport->workqueue, &sport->rxwork);
+}
+
+static int i3c_open(struct tty_struct *tty, struct file *filp)
+{
+ struct ttyi3c_port *sport = tty->driver_data;
+
+ return tty_port_open(&sport->port, tty, filp);
+}
+
+static void i3c_close(struct tty_struct *tty, struct file *filp)
+{
+ struct ttyi3c_port *sport = tty->driver_data;
+
+ if (!sport)
+ return;
+
+ tty_port_close(tty->port, tty, filp);
+}
+
+static void i3c_wait_until_sent(struct tty_struct *tty, int timeout)
+{
+ struct ttyi3c_port *sport = tty->driver_data;
+
+ wait_for_completion_timeout(&sport->txcomplete, timeout);
+ reinit_completion(&sport->txcomplete);
+}
+
+static const struct tty_operations i3c_tty_ops = {
+ .install = i3c_install,
+ .open = i3c_open,
+ .close = i3c_close,
+ .write = i3c_write,
+ .put_char = i3c_put_char,
+ .flush_chars = i3c_flush_chars,
+ .write_room = i3c_write_room,
+ .throttle = i3c_throttle,
+ .unthrottle = i3c_unthrottle,
+ .wait_until_sent = i3c_wait_until_sent,
+};
+
+static void i3c_controller_irq_handler(struct i3c_device *dev,
+ const struct i3c_ibi_payload *payload)
+{
+ struct ttyi3c_port *sport = dev_get_drvdata(&dev->dev);
+
+ queue_work(sport->workqueue, &sport->rxwork);
+}
+
+static void tty_i3c_rxwork(struct work_struct *work)
+{
+ struct ttyi3c_port *sport = container_of(work, struct ttyi3c_port, rxwork);
+ struct i3c_priv_xfer xfers;
+ int retry = I3C_TTY_RETRY;
+ u16 status = BIT(0);
+
+ do {
+ memset(&xfers, 0, sizeof(xfers));
+ xfers.data.in = sport->buffer;
+ xfers.len = I3C_TTY_TRANS_SIZE;
+ xfers.rnw = 1;
+
+ if (I3C_TTY_RX_STOP & atomic_read(&sport->status))
+ break;
+
+ i3c_device_do_priv_xfers(sport->i3cdev, &xfers, 1);
+
+ if (xfers.actual_len) {
+ tty_insert_flip_string(&sport->port, sport->buffer, xfers.actual_len);
+ retry = 20;
+ continue;
+ } else {
+ status = BIT(0);
+ i3c_device_getstatus_format1(sport->i3cdev, &status);
+ /*
+ * Target side need some time to fill data into fifo. Target side may not
+ * have hardware update status in real time. Software update status always
+ * need some delays.
+ *
+ * Generally, target side have cicular buffer in memory, it will be moved
+ * into FIFO by CPU or DMA. 'status' just show if cicular buffer empty. But
+ * there are gap, espcially CPU have not response irq to fill FIFO in time.
+ * So xfers.actual will be zero, wait for little time to avoid flood
+ * transfer in i3c bus.
+ */
+ usleep_range(I3C_TTY_YIELD_US, 10 * I3C_TTY_YIELD_US);
+ retry--;
+ }
+
+ } while (retry && (status & BIT(0)));
+
+ tty_flip_buffer_push(&sport->port);
+}
+
+static void tty_i3c_txwork(struct work_struct *work)
+{
+ struct ttyi3c_port *sport = container_of(work, struct ttyi3c_port, txwork);
+ struct circ_buf *circ = &sport->xmit;
+ int cnt = CIRC_CNT(circ->head, circ->tail, UART_XMIT_SIZE);
+ struct i3c_priv_xfer xfers;
+ int retry = I3C_TTY_RETRY;
+ unsigned long flags;
+ int actual;
+ int ret;
+
+ while (cnt > 0 && retry) {
+ xfers.rnw = 0;
+ xfers.len = CIRC_CNT_TO_END(circ->head, circ->tail, UART_XMIT_SIZE);
+ xfers.len = min_t(u16, 16, xfers.len);
+ xfers.data.out = circ->buf + circ->tail;
+
+ ret = i3c_device_do_priv_xfers(sport->i3cdev, &xfers, 1);
+ if (ret) {
+ /*
+ * Target side may not move data out of FIFO. delay can't resolve problem,
+ * just reduce some possiblity. Target can't end I3C SDR mode write
+ * transfer, discard data is reasonable when FIFO overrun.
+ */
+ usleep_range(I3C_TTY_YIELD_US, 10 * I3C_TTY_YIELD_US);
+ retry--;
+ } else {
+ retry = 0;
+ }
+
+ actual = xfers.len;
+
+ circ->tail = (circ->tail + actual) & (UART_XMIT_SIZE - 1);
+
+ if (CIRC_CNT(circ->head, circ->tail, UART_XMIT_SIZE) < WAKEUP_CHARS)
+ tty_port_tty_wakeup(&sport->port);
+
+ cnt = CIRC_CNT(circ->head, circ->tail, UART_XMIT_SIZE);
+ }
+
+ spin_lock_irqsave(&sport->xlock, flags);
+ if (circ->head == circ->tail)
+ complete(&sport->txcomplete);
+ spin_unlock_irqrestore(&sport->xlock, flags);
+}
+
+static int i3c_probe(struct i3c_device *i3cdev)
+{
+ struct ttyi3c_port *port;
+ struct device *tty_dev;
+ struct i3c_ibi_setup req;
+ int minor;
+ int ret;
+
+ port = devm_kzalloc(&i3cdev->dev, sizeof(*port), GFP_KERNEL);
+ if (!port)
+ return -ENOMEM;
+
+ port->i3cdev = i3cdev;
+ port->buffer = devm_kzalloc(&i3cdev->dev, UART_XMIT_SIZE, GFP_KERNEL);
+ if (!port->buffer)
+ return -ENOMEM;
+
+ port->xmit.buf = devm_kzalloc(&i3cdev->dev, UART_XMIT_SIZE, GFP_KERNEL);
+ if (!port->xmit.buf)
+ return -ENOMEM;
+
+ dev_set_drvdata(&i3cdev->dev, port);
+
+ req.max_payload_len = 8;
+ req.num_slots = 4;
+ req.handler = &i3c_controller_irq_handler;
+
+ ret = i3c_device_request_ibi(i3cdev, &req);
+ if (ret)
+ return -EINVAL;
+
+ mutex_lock(&i3c_tty_minors_lock);
+ minor = idr_alloc(&i3c_tty_minors, port, 0, I3C_TTY_MINORS, GFP_KERNEL);
+ mutex_unlock(&i3c_tty_minors_lock);
+
+ if (minor < 0)
+ return -EINVAL;
+
+ spin_lock_init(&port->xlock);
+ INIT_WORK(&port->txwork, tty_i3c_txwork);
+ INIT_WORK(&port->rxwork, tty_i3c_rxwork);
+ init_completion(&port->txcomplete);
+
+ port->workqueue = alloc_workqueue("%s", 0, 0, dev_name(&i3cdev->dev));
+ if (!port->workqueue)
+ return -ENOMEM;
+
+ tty_port_init(&port->port);
+ port->port.ops = &i3c_port_ops;
+
+ tty_dev = tty_port_register_device(&port->port, i3c_tty_driver, minor,
+ &i3cdev->dev);
+ if (IS_ERR(tty_dev)) {
+ destroy_workqueue(port->workqueue);
+ return PTR_ERR(tty_dev);
+ }
+
+ port->minor = minor;
+
+ return 0;
+}
+
+void i3c_remove(struct i3c_device *dev)
+{
+ struct ttyi3c_port *sport = dev_get_drvdata(&dev->dev);
+
+ tty_port_unregister_device(&sport->port, i3c_tty_driver, sport->minor);
+ cancel_work_sync(&sport->txwork);
+ destroy_workqueue(sport->workqueue);
+}
+
+static struct i3c_driver i3c_driver = {
+ .driver = {
+ .name = "ttyi3c",
+ },
+ .probe = i3c_probe,
+ .remove = i3c_remove,
+ .id_table = i3c_ids,
+};
+
+static int __init i3c_tty_init(void)
+{
+ int ret;
+
+ i3c_tty_driver = tty_alloc_driver(I3C_TTY_MINORS,
+ TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV);
+
+ if (IS_ERR(i3c_tty_driver))
+ return PTR_ERR(i3c_tty_driver);
+
+ i3c_tty_driver->driver_name = "ttyI3C";
+ i3c_tty_driver->name = "ttyI3C";
+ i3c_tty_driver->minor_start = 0,
+ i3c_tty_driver->type = TTY_DRIVER_TYPE_SERIAL,
+ i3c_tty_driver->subtype = SERIAL_TYPE_NORMAL,
+ i3c_tty_driver->init_termios = tty_std_termios;
+ i3c_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL |
+ CLOCAL;
+ i3c_tty_driver->init_termios.c_lflag = 0;
+
+ tty_set_operations(i3c_tty_driver, &i3c_tty_ops);
+
+ ret = tty_register_driver(i3c_tty_driver);
+ if (ret) {
+ tty_driver_kref_put(i3c_tty_driver);
+ return ret;
+ }
+
+ ret = i3c_driver_register(&i3c_driver);
+ if (ret) {
+ tty_unregister_driver(i3c_tty_driver);
+ tty_driver_kref_put(i3c_tty_driver);
+ }
+
+ return ret;
+}
+
+static void __exit i3c_tty_exit(void)
+{
+ i3c_driver_unregister(&i3c_driver);
+ tty_unregister_driver(i3c_tty_driver);
+ tty_driver_kref_put(i3c_tty_driver);
+ idr_destroy(&i3c_tty_minors);
+}
+
+module_init(i3c_tty_init);
+module_exit(i3c_tty_exit);
+
+MODULE_LICENSE("GPL");
--
2.34.1
On 18. 10. 23, 23:11, Frank Li wrote:
> Add new driver to allow tty over i3c master.
What is it good for? Why we should consider this for inclusion at all?
> Signed-off-by: Frank Li <[email protected]>
> ---
>
> Notes:
> This patch depend on
> https://lore.kernel.org/imx/[email protected]/T/#t
>
> drivers/tty/Kconfig | 8 +
> drivers/tty/Makefile | 1 +
> drivers/tty/i3c_tty.c | 466 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 475 insertions(+)
> create mode 100644 drivers/tty/i3c_tty.c
>
> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index 5646dc6242cd9..6d91fe6a211a1 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -412,6 +412,14 @@ config RPMSG_TTY
> To compile this driver as a module, choose M here: the module will be
> called rpmsg_tty.
>
> +config I3C_TTY
> + tristate "tty over i3c"
"TTY over I3C"
> + depends on I3C
> + help
> + Select this options if you'd like use tty over I3C master controller
TTY and add a period.
> --- /dev/null
> +++ b/drivers/tty/i3c_tty.c
> @@ -0,0 +1,466 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2023 NXP.
> + *
> + * Author: Frank Li <[email protected]>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/i3c/device.h>
> +#include <linux/i3c/master.h>
> +#include <linux/slab.h>
> +#include <linux/console.h>
> +#include <linux/serial_core.h>
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +#include <linux/tty_flip.h>
> +
> +static DEFINE_IDR(i3c_tty_minors);
> +static DEFINE_MUTEX(i3c_tty_minors_lock);
> +
> +static struct tty_driver *i3c_tty_driver;
> +
> +#define I3C_TTY_MINORS 256
> +#define I3C_TTY_TRANS_SIZE 16
> +#define I3C_TTY_RX_STOP BIT(0)
> +#define I3C_TTY_RETRY 20
> +#define I3C_TTY_YIELD_US 100
> +
> +struct ttyi3c_port {
> + struct tty_port port;
> + int minor;
> + unsigned int txfifo_size;
> + unsigned int rxfifo_size;
> + struct circ_buf xmit;
Why can't you use port.xmit_fifo?
> + spinlock_t xlock; /* protect xmit */
> + void *buffer;
> + struct i3c_device *i3cdev;
> + struct work_struct txwork;
> + struct work_struct rxwork;
> + struct completion txcomplete;
> + struct workqueue_struct *workqueue;
Why do you need a special wq? And even, why per port?
> + atomic_t status;
> +};
> +
> +static const struct i3c_device_id i3c_ids[] = {
> + I3C_DEVICE(0x011B, 0x1000, NULL),
> + { /* sentinel */ },
> +};
> +
> +static int i3c_port_activate(struct tty_port *port, struct tty_struct *tty)
> +{
> + struct ttyi3c_port *sport = container_of(port, struct ttyi3c_port, port);
> +
> + atomic_set(&sport->status, 0);
> +
> + return i3c_device_enable_ibi(sport->i3cdev);
> +}
> +
> +static void i3c_port_shutdown(struct tty_port *port)
> +{
> + struct ttyi3c_port *sport =
> + container_of(port, struct ttyi3c_port, port);
> +
> + i3c_device_disable_ibi(sport->i3cdev);
> +}
> +
> +static void i3c_port_destruct(struct tty_port *port)
> +{
> + struct ttyi3c_port *sport =
> + container_of(port, struct ttyi3c_port, port);
> +
> + mutex_lock(&i3c_tty_minors_lock);
> + idr_remove(&i3c_tty_minors, sport->minor);
> + mutex_unlock(&i3c_tty_minors_lock);
> +}
> +
> +static const struct tty_port_operations i3c_port_ops = {
> + .shutdown = i3c_port_shutdown,
> + .activate = i3c_port_activate,
> + .destruct = i3c_port_destruct,
> +};
> +
> +static struct ttyi3c_port *i3c_get_by_minor(unsigned int minor)
> +{
> + struct ttyi3c_port *sport;
> +
> + mutex_lock(&i3c_tty_minors_lock);
> + sport = idr_find(&i3c_tty_minors, minor);
> + mutex_unlock(&i3c_tty_minors_lock);
> +
> + return sport;
> +}
> +
> +static int i3c_install(struct tty_driver *driver, struct tty_struct *tty)
> +{
> + struct ttyi3c_port *sport;
> + int ret;
> +
> + sport = i3c_get_by_minor(tty->index);
> + if (!sport)
> + return -ENODEV;
> +
> + ret = tty_standard_install(driver, tty);
> + if (ret)
> + return ret;
> +
> + tty->driver_data = sport;
> +
> + return 0;
> +}
You don't need this at all. (Watch for XXX marks below.)
> +static ssize_t i3c_write(struct tty_struct *tty, const unsigned char *buf, size_t count)
> +{
> + struct ttyi3c_port *sport = tty->driver_data;
> + struct circ_buf *circ = &sport->xmit;
> + unsigned long flags;
> + int c, ret = 0;
> +
> + spin_lock_irqsave(&sport->xlock, flags);
> + while (1) {
> + c = CIRC_SPACE_TO_END(circ->head, circ->tail, UART_XMIT_SIZE);
> + if (count < c)
> + c = count;
> + if (c <= 0)
> + break;
> +
> + memcpy(circ->buf + circ->head, buf, c);
> + circ->head = (circ->head + c) & (UART_XMIT_SIZE - 1);
> + buf += c;
> + count -= c;
> + ret += c;
> + }
> + spin_unlock_irqrestore(&sport->xlock, flags);
With kfifo, all this would be one line, right?
> +
> + if (circ->head != circ->tail)
> + queue_work(sport->workqueue, &sport->txwork);
> +
> + return ret;
> +}
> +
> +static int i3c_put_char(struct tty_struct *tty, unsigned char ch)
> +{
> + struct ttyi3c_port *sport = tty->driver_data;
> + struct circ_buf *circ = &sport->xmit;
> + unsigned long flags;
> + int ret = 0;
> +
> + spin_lock_irqsave(&sport->xlock, flags);
> +
> + if (sport && CIRC_SPACE(circ->head, circ->tail, UART_XMIT_SIZE) != 0) {
> + circ->buf[circ->head] = ch;
> + circ->head = (circ->head + 1) & (UART_XMIT_SIZE - 1);
> + ret = 1;
> + }
> +
> + spin_unlock_irqrestore(&sport->xlock, flags);
> +
> + return ret;
> +}
> +
> +static void i3c_flush_chars(struct tty_struct *tty)
> +{
> + struct ttyi3c_port *sport = tty->driver_data;
> +
> + queue_work(sport->workqueue, &sport->txwork);
> +}
> +
> +static unsigned int i3c_write_room(struct tty_struct *tty)
> +{
> + struct ttyi3c_port *sport = tty->driver_data;
> + struct circ_buf *circ = &sport->xmit;
> + unsigned long flags;
> + int ret = 0;
> +
> + spin_lock_irqsave(&sport->xlock, flags);
> + ret = CIRC_SPACE(circ->head, circ->tail, UART_XMIT_SIZE);
> + spin_unlock_irqrestore(&sport->xlock, flags);
> +
> + return ret;
> +}
> +
> +static void i3c_throttle(struct tty_struct *tty)
> +{
> + struct ttyi3c_port *sport = tty->driver_data;
> +
> + atomic_or(I3C_TTY_RX_STOP, &sport->status);
> +}
> +
> +static void i3c_unthrottle(struct tty_struct *tty)
> +{
> + struct ttyi3c_port *sport = tty->driver_data;
> +
> + atomic_andnot(I3C_TTY_RX_STOP, &sport->status);
> +
> + queue_work(sport->workqueue, &sport->rxwork);
> +}
> +
> +static int i3c_open(struct tty_struct *tty, struct file *filp)
> +{
> + struct ttyi3c_port *sport = tty->driver_data;
XXX Here, you can simply do:
struct ttyi3c_port *sport = container_of(tty->port, struct ttyi3c_port,
port);
tty->driver_data = sport;
> + return tty_port_open(&sport->port, tty, filp);
> +}
> +
> +static void i3c_close(struct tty_struct *tty, struct file *filp)
> +{
> + struct ttyi3c_port *sport = tty->driver_data;
> +
> + if (!sport)
> + return;
How can that happen?
> + tty_port_close(tty->port, tty, filp);
> +}
> +
> +static void i3c_wait_until_sent(struct tty_struct *tty, int timeout)
> +{
> + struct ttyi3c_port *sport = tty->driver_data;
> +
> + wait_for_completion_timeout(&sport->txcomplete, timeout);
> + reinit_completion(&sport->txcomplete);
Does this work in parallel?
> +}
> +
> +static const struct tty_operations i3c_tty_ops = {
> + .install = i3c_install,
> + .open = i3c_open,
> + .close = i3c_close,
> + .write = i3c_write,
> + .put_char = i3c_put_char,
> + .flush_chars = i3c_flush_chars,
> + .write_room = i3c_write_room,
> + .throttle = i3c_throttle,
> + .unthrottle = i3c_unthrottle,
> + .wait_until_sent = i3c_wait_until_sent,
> +};
> +
> +static void i3c_controller_irq_handler(struct i3c_device *dev,
> + const struct i3c_ibi_payload *payload)
> +{
> + struct ttyi3c_port *sport = dev_get_drvdata(&dev->dev);
> +
> + queue_work(sport->workqueue, &sport->rxwork);
Doesn't ibi provide threaded irqs? Oh, wait, i3c_master_handle_ibi() is
already a work!
> +}
> +
> +static void tty_i3c_rxwork(struct work_struct *work)
> +{
> + struct ttyi3c_port *sport = container_of(work, struct ttyi3c_port, rxwork);
> + struct i3c_priv_xfer xfers;
> + int retry = I3C_TTY_RETRY;
> + u16 status = BIT(0);
> +
> + do {
> + memset(&xfers, 0, sizeof(xfers));
> + xfers.data.in = sport->buffer;
> + xfers.len = I3C_TTY_TRANS_SIZE;
> + xfers.rnw = 1;
> +
> + if (I3C_TTY_RX_STOP & atomic_read(&sport->status))
Hmm, why not much simpler (and yet atomic) {set,test,clear}_bit()?
> + break;
> +
> + i3c_device_do_priv_xfers(sport->i3cdev, &xfers, 1);
> +
> + if (xfers.actual_len) {
> + tty_insert_flip_string(&sport->port, sport->buffer, xfers.actual_len);
This can fail.
> + retry = 20;
> + continue;
> + } else {
"} else {" uneeded.
> + status = BIT(0);
> + i3c_device_getstatus_format1(sport->i3cdev, &status);
> + /*
> + * Target side need some time to fill data into fifo. Target side may not
"needs"
> + * have hardware update status in real time. Software update status always
> + * need some delays.
"needs"
> + *
> + * Generally, target side have cicular buffer in memory, it will be moved
"circular" all around.
> + * into FIFO by CPU or DMA. 'status' just show if cicular buffer empty. But
> + * there are gap, espcially CPU have not response irq to fill FIFO in time.
espcially
> + * So xfers.actual will be zero, wait for little time to avoid flood
> + * transfer in i3c bus.
> + */
> + usleep_range(I3C_TTY_YIELD_US, 10 * I3C_TTY_YIELD_US);
> + retry--;
> + }
> +
> + } while (retry && (status & BIT(0)));
> +
> + tty_flip_buffer_push(&sport->port);
> +}
> +
> +static void tty_i3c_txwork(struct work_struct *work)
> +{
> + struct ttyi3c_port *sport = container_of(work, struct ttyi3c_port, txwork);
> + struct circ_buf *circ = &sport->xmit;
> + int cnt = CIRC_CNT(circ->head, circ->tail, UART_XMIT_SIZE);
> + struct i3c_priv_xfer xfers;
> + int retry = I3C_TTY_RETRY;
> + unsigned long flags;
> + int actual;
> + int ret;
> +
> + while (cnt > 0 && retry) {
> + xfers.rnw = 0;
> + xfers.len = CIRC_CNT_TO_END(circ->head, circ->tail, UART_XMIT_SIZE);
> + xfers.len = min_t(u16, 16, xfers.len);
> + xfers.data.out = circ->buf + circ->tail;
> +
> + ret = i3c_device_do_priv_xfers(sport->i3cdev, &xfers, 1);
> + if (ret) {
> + /*
> + * Target side may not move data out of FIFO. delay can't resolve problem,
> + * just reduce some possiblity. Target can't end I3C SDR mode write
> + * transfer, discard data is reasonable when FIFO overrun.
> + */
> + usleep_range(I3C_TTY_YIELD_US, 10 * I3C_TTY_YIELD_US);
> + retry--;
> + } else {
> + retry = 0;
> + }
> +
> + actual = xfers.len;
> +
> + circ->tail = (circ->tail + actual) & (UART_XMIT_SIZE - 1);
> +
> + if (CIRC_CNT(circ->head, circ->tail, UART_XMIT_SIZE) < WAKEUP_CHARS)
> + tty_port_tty_wakeup(&sport->port);
> +
> + cnt = CIRC_CNT(circ->head, circ->tail, UART_XMIT_SIZE);
> + }
> +
> + spin_lock_irqsave(&sport->xlock, flags);
> + if (circ->head == circ->tail)
> + complete(&sport->txcomplete);
> + spin_unlock_irqrestore(&sport->xlock, flags);
> +}
> +
> +static int i3c_probe(struct i3c_device *i3cdev)
> +{
> + struct ttyi3c_port *port;
> + struct device *tty_dev;
> + struct i3c_ibi_setup req;
> + int minor;
> + int ret;
> +
> + port = devm_kzalloc(&i3cdev->dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> +
> + port->i3cdev = i3cdev;
> + port->buffer = devm_kzalloc(&i3cdev->dev, UART_XMIT_SIZE, GFP_KERNEL);
> + if (!port->buffer)
> + return -ENOMEM;
> +
> + port->xmit.buf = devm_kzalloc(&i3cdev->dev, UART_XMIT_SIZE, GFP_KERNEL);
> + if (!port->xmit.buf)
> + return -ENOMEM;
You allocate two pages even if you never use the device?
> + dev_set_drvdata(&i3cdev->dev, port);
> +
> + req.max_payload_len = 8;
> + req.num_slots = 4;
> + req.handler = &i3c_controller_irq_handler;
> +
> + ret = i3c_device_request_ibi(i3cdev, &req);
> + if (ret)
> + return -EINVAL;
> +
> + mutex_lock(&i3c_tty_minors_lock);
> + minor = idr_alloc(&i3c_tty_minors, port, 0, I3C_TTY_MINORS, GFP_KERNEL);
> + mutex_unlock(&i3c_tty_minors_lock);
> +
> + if (minor < 0)
> + return -EINVAL;
> +
> + spin_lock_init(&port->xlock);
> + INIT_WORK(&port->txwork, tty_i3c_txwork);
> + INIT_WORK(&port->rxwork, tty_i3c_rxwork);
> + init_completion(&port->txcomplete);
> +
> + port->workqueue = alloc_workqueue("%s", 0, 0, dev_name(&i3cdev->dev));
> + if (!port->workqueue)
> + return -ENOMEM;
> +
> + tty_port_init(&port->port);
> + port->port.ops = &i3c_port_ops;
> +
> + tty_dev = tty_port_register_device(&port->port, i3c_tty_driver, minor,
> + &i3cdev->dev);
> + if (IS_ERR(tty_dev)) {
> + destroy_workqueue(port->workqueue);
What about tty_port_put() (it drops the idr too)? And free ibi?
> + return PTR_ERR(tty_dev);
> + }
> +
> + port->minor = minor;
> +
> + return 0;
> +}
> +
> +void i3c_remove(struct i3c_device *dev)
> +{
> + struct ttyi3c_port *sport = dev_get_drvdata(&dev->dev);
> +
> + tty_port_unregister_device(&sport->port, i3c_tty_driver, sport->minor);
> + cancel_work_sync(&sport->txwork);
> + destroy_workqueue(sport->workqueue);
The same as above.
> +}
> +
> +static struct i3c_driver i3c_driver = {
> + .driver = {
> + .name = "ttyi3c",
> + },
> + .probe = i3c_probe,
> + .remove = i3c_remove,
> + .id_table = i3c_ids,
> +};
> +
> +static int __init i3c_tty_init(void)
> +{
> + int ret;
> +
> + i3c_tty_driver = tty_alloc_driver(I3C_TTY_MINORS,
> + TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV);
> +
> + if (IS_ERR(i3c_tty_driver))
> + return PTR_ERR(i3c_tty_driver);
> +
> + i3c_tty_driver->driver_name = "ttyI3C";
> + i3c_tty_driver->name = "ttyI3C";
> + i3c_tty_driver->minor_start = 0,
> + i3c_tty_driver->type = TTY_DRIVER_TYPE_SERIAL,
> + i3c_tty_driver->subtype = SERIAL_TYPE_NORMAL,
> + i3c_tty_driver->init_termios = tty_std_termios;
> + i3c_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL |
> + CLOCAL;
> + i3c_tty_driver->init_termios.c_lflag = 0;
> +
> + tty_set_operations(i3c_tty_driver, &i3c_tty_ops);
> +
> + ret = tty_register_driver(i3c_tty_driver);
> + if (ret) {
> + tty_driver_kref_put(i3c_tty_driver);
> + return ret;
> + }
> +
> + ret = i3c_driver_register(&i3c_driver);
> + if (ret) {
> + tty_unregister_driver(i3c_tty_driver);
> + tty_driver_kref_put(i3c_tty_driver);
Use goto + fail paths. And in i3c_probe() too.
> + }
> +
> + return ret;
> +}
regards,
--
js
suse labs
On Thu, Oct 19, 2023 at 09:12:58AM +0200, Jiri Slaby wrote:
> On 18. 10. 23, 23:11, Frank Li wrote:
> > Add new driver to allow tty over i3c master.
>
> What is it good for? Why we should consider this for inclusion at all?
UART console at least need TX/RX two pin. If we can use I3C as linux
console, the board will save two PAD because we always use I3C to connected
into some sensor or PMIC chip.
Some small chip package, PIN is very valuable.
>
> > Signed-off-by: Frank Li <[email protected]>
> > ---
> >
> > Notes:
> > This patch depend on
> > https://lore.kernel.org/imx/[email protected]/T/#t
> >
> > drivers/tty/Kconfig | 8 +
> > drivers/tty/Makefile | 1 +
> > drivers/tty/i3c_tty.c | 466 ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 475 insertions(+)
> > create mode 100644 drivers/tty/i3c_tty.c
> >
> > diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> > index 5646dc6242cd9..6d91fe6a211a1 100644
> > --- a/drivers/tty/Kconfig
> > +++ b/drivers/tty/Kconfig
> > @@ -412,6 +412,14 @@ config RPMSG_TTY
> > To compile this driver as a module, choose M here: the module will be
> > called rpmsg_tty.
> > +config I3C_TTY
> > + tristate "tty over i3c"
>
> "TTY over I3C"
>
> > + depends on I3C
> > + help
> > + Select this options if you'd like use tty over I3C master controller
>
> TTY and add a period.
>
> > --- /dev/null
> > +++ b/drivers/tty/i3c_tty.c
> > @@ -0,0 +1,466 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2023 NXP.
> > + *
> > + * Author: Frank Li <[email protected]>
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/i3c/device.h>
> > +#include <linux/i3c/master.h>
> > +#include <linux/slab.h>
> > +#include <linux/console.h>
> > +#include <linux/serial_core.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/tty_flip.h>
> > +
> > +static DEFINE_IDR(i3c_tty_minors);
> > +static DEFINE_MUTEX(i3c_tty_minors_lock);
> > +
> > +static struct tty_driver *i3c_tty_driver;
> > +
> > +#define I3C_TTY_MINORS 256
> > +#define I3C_TTY_TRANS_SIZE 16
> > +#define I3C_TTY_RX_STOP BIT(0)
> > +#define I3C_TTY_RETRY 20
> > +#define I3C_TTY_YIELD_US 100
> > +
> > +struct ttyi3c_port {
> > + struct tty_port port;
> > + int minor;
> > + unsigned int txfifo_size;
> > + unsigned int rxfifo_size;
> > + struct circ_buf xmit;
>
> Why can't you use port.xmit_fifo?
>
> > + spinlock_t xlock; /* protect xmit */
> > + void *buffer;
> > + struct i3c_device *i3cdev;
> > + struct work_struct txwork;
> > + struct work_struct rxwork;
> > + struct completion txcomplete;
> > + struct workqueue_struct *workqueue;
>
> Why do you need a special wq? And even, why per port?
You are right! Do you any 'common wq' perfered?
>
> > + atomic_t status;
> > +};
> > +
> > +static const struct i3c_device_id i3c_ids[] = {
> > + I3C_DEVICE(0x011B, 0x1000, NULL),
> > + { /* sentinel */ },
> > +};
> > +
> > +static int i3c_port_activate(struct tty_port *port, struct tty_struct *tty)
> > +{
> > + struct ttyi3c_port *sport = container_of(port, struct ttyi3c_port, port);
> > +
> > + atomic_set(&sport->status, 0);
> > +
> > + return i3c_device_enable_ibi(sport->i3cdev);
> > +}
> > +
> > +static void i3c_port_shutdown(struct tty_port *port)
> > +{
> > + struct ttyi3c_port *sport =
> > + container_of(port, struct ttyi3c_port, port);
> > +
> > + i3c_device_disable_ibi(sport->i3cdev);
> > +}
> > +
> > +static void i3c_port_destruct(struct tty_port *port)
> > +{
> > + struct ttyi3c_port *sport =
> > + container_of(port, struct ttyi3c_port, port);
> > +
> > + mutex_lock(&i3c_tty_minors_lock);
> > + idr_remove(&i3c_tty_minors, sport->minor);
> > + mutex_unlock(&i3c_tty_minors_lock);
> > +}
> > +
> > +static const struct tty_port_operations i3c_port_ops = {
> > + .shutdown = i3c_port_shutdown,
> > + .activate = i3c_port_activate,
> > + .destruct = i3c_port_destruct,
> > +};
> > +
> > +static struct ttyi3c_port *i3c_get_by_minor(unsigned int minor)
> > +{
> > + struct ttyi3c_port *sport;
> > +
> > + mutex_lock(&i3c_tty_minors_lock);
> > + sport = idr_find(&i3c_tty_minors, minor);
> > + mutex_unlock(&i3c_tty_minors_lock);
> > +
> > + return sport;
> > +}
> > +
> > +static int i3c_install(struct tty_driver *driver, struct tty_struct *tty)
> > +{
> > + struct ttyi3c_port *sport;
> > + int ret;
> > +
> > + sport = i3c_get_by_minor(tty->index);
> > + if (!sport)
> > + return -ENODEV;
> > +
> > + ret = tty_standard_install(driver, tty);
> > + if (ret)
> > + return ret;
> > +
> > + tty->driver_data = sport;
> > +
> > + return 0;
> > +}
>
> You don't need this at all. (Watch for XXX marks below.)
>
> > +static ssize_t i3c_write(struct tty_struct *tty, const unsigned char *buf, size_t count)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > + struct circ_buf *circ = &sport->xmit;
> > + unsigned long flags;
> > + int c, ret = 0;
> > +
> > + spin_lock_irqsave(&sport->xlock, flags);
> > + while (1) {
> > + c = CIRC_SPACE_TO_END(circ->head, circ->tail, UART_XMIT_SIZE);
> > + if (count < c)
> > + c = count;
> > + if (c <= 0)
> > + break;
> > +
> > + memcpy(circ->buf + circ->head, buf, c);
> > + circ->head = (circ->head + c) & (UART_XMIT_SIZE - 1);
> > + buf += c;
> > + count -= c;
> > + ret += c;
> > + }
> > + spin_unlock_irqrestore(&sport->xlock, flags);
>
> With kfifo, all this would be one line, right?
>
> > +
> > + if (circ->head != circ->tail)
> > + queue_work(sport->workqueue, &sport->txwork);
> > +
> > + return ret;
> > +}
> > +
> > +static int i3c_put_char(struct tty_struct *tty, unsigned char ch)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > + struct circ_buf *circ = &sport->xmit;
> > + unsigned long flags;
> > + int ret = 0;
> > +
> > + spin_lock_irqsave(&sport->xlock, flags);
> > +
> > + if (sport && CIRC_SPACE(circ->head, circ->tail, UART_XMIT_SIZE) != 0) {
> > + circ->buf[circ->head] = ch;
> > + circ->head = (circ->head + 1) & (UART_XMIT_SIZE - 1);
> > + ret = 1;
> > + }
> > +
> > + spin_unlock_irqrestore(&sport->xlock, flags);
> > +
> > + return ret;
> > +}
> > +
> > +static void i3c_flush_chars(struct tty_struct *tty)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > +
> > + queue_work(sport->workqueue, &sport->txwork);
> > +}
> > +
> > +static unsigned int i3c_write_room(struct tty_struct *tty)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > + struct circ_buf *circ = &sport->xmit;
> > + unsigned long flags;
> > + int ret = 0;
> > +
> > + spin_lock_irqsave(&sport->xlock, flags);
> > + ret = CIRC_SPACE(circ->head, circ->tail, UART_XMIT_SIZE);
> > + spin_unlock_irqrestore(&sport->xlock, flags);
> > +
> > + return ret;
> > +}
> > +
> > +static void i3c_throttle(struct tty_struct *tty)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > +
> > + atomic_or(I3C_TTY_RX_STOP, &sport->status);
> > +}
> > +
> > +static void i3c_unthrottle(struct tty_struct *tty)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > +
> > + atomic_andnot(I3C_TTY_RX_STOP, &sport->status);
> > +
> > + queue_work(sport->workqueue, &sport->rxwork);
> > +}
> > +
> > +static int i3c_open(struct tty_struct *tty, struct file *filp)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
>
> XXX Here, you can simply do:
>
> struct ttyi3c_port *sport = container_of(tty->port, struct ttyi3c_port,
> port);
>
> tty->driver_data = sport;
>
>
> > + return tty_port_open(&sport->port, tty, filp);
> > +}
> > +
> > +static void i3c_close(struct tty_struct *tty, struct file *filp)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > +
> > + if (!sport)
> > + return;
>
> How can that happen?
>
> > + tty_port_close(tty->port, tty, filp);
> > +}
> > +
> > +static void i3c_wait_until_sent(struct tty_struct *tty, int timeout)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > +
> > + wait_for_completion_timeout(&sport->txcomplete, timeout);
> > + reinit_completion(&sport->txcomplete);
>
> Does this work in parallel?
I am not clear! What's your concern? txcmplete should be by tx work thread.
>
> > +}
> > +
> > +static const struct tty_operations i3c_tty_ops = {
> > + .install = i3c_install,
> > + .open = i3c_open,
> > + .close = i3c_close,
> > + .write = i3c_write,
> > + .put_char = i3c_put_char,
> > + .flush_chars = i3c_flush_chars,
> > + .write_room = i3c_write_room,
> > + .throttle = i3c_throttle,
> > + .unthrottle = i3c_unthrottle,
> > + .wait_until_sent = i3c_wait_until_sent,
> > +};
> > +
> > +static void i3c_controller_irq_handler(struct i3c_device *dev,
> > + const struct i3c_ibi_payload *payload)
> > +{
> > + struct ttyi3c_port *sport = dev_get_drvdata(&dev->dev);
> > +
> > + queue_work(sport->workqueue, &sport->rxwork);
>
> Doesn't ibi provide threaded irqs? Oh, wait, i3c_master_handle_ibi() is
> already a work!
Yes, but I need stop rxwork when i3c_unthrottle and start rxwork at
i3c_throttle.
I am not sure if throttle/unthrottle allow sleep. I refer other drivers,
most just wakeup or disable some simple works.
>
> > +}
> > +
> > +static void tty_i3c_rxwork(struct work_struct *work)
> > +{
> > + struct ttyi3c_port *sport = container_of(work, struct ttyi3c_port, rxwork);
> > + struct i3c_priv_xfer xfers;
> > + int retry = I3C_TTY_RETRY;
> > + u16 status = BIT(0);
> > +
> > + do {
> > + memset(&xfers, 0, sizeof(xfers));
> > + xfers.data.in = sport->buffer;
> > + xfers.len = I3C_TTY_TRANS_SIZE;
> > + xfers.rnw = 1;
> > +
> > + if (I3C_TTY_RX_STOP & atomic_read(&sport->status))
>
> Hmm, why not much simpler (and yet atomic) {set,test,clear}_bit()?
>
> > + break;
> > +
> > + i3c_device_do_priv_xfers(sport->i3cdev, &xfers, 1);
> > +
> > + if (xfers.actual_len) {
> > + tty_insert_flip_string(&sport->port, sport->buffer, xfers.actual_len);
>
> This can fail.
>
> > + retry = 20;
> > + continue;
> > + } else {
>
> "} else {" uneeded.
>
> > + status = BIT(0);
> > + i3c_device_getstatus_format1(sport->i3cdev, &status);
> > + /*
> > + * Target side need some time to fill data into fifo. Target side may not
>
> "needs"
>
> > + * have hardware update status in real time. Software update status always
> > + * need some delays.
>
> "needs"
>
> > + *
> > + * Generally, target side have cicular buffer in memory, it will be moved
> "circular" all around.
>
> > + * into FIFO by CPU or DMA. 'status' just show if cicular buffer empty. But
> > + * there are gap, espcially CPU have not response irq to fill FIFO in time.
>
> espcially
>
> > + * So xfers.actual will be zero, wait for little time to avoid flood
> > + * transfer in i3c bus.
> > + */
> > + usleep_range(I3C_TTY_YIELD_US, 10 * I3C_TTY_YIELD_US);
> > + retry--;
> > + }
> > +
> > + } while (retry && (status & BIT(0)));
> > +
> > + tty_flip_buffer_push(&sport->port);
> > +}
> > +
> > +static void tty_i3c_txwork(struct work_struct *work)
> > +{
> > + struct ttyi3c_port *sport = container_of(work, struct ttyi3c_port, txwork);
> > + struct circ_buf *circ = &sport->xmit;
> > + int cnt = CIRC_CNT(circ->head, circ->tail, UART_XMIT_SIZE);
> > + struct i3c_priv_xfer xfers;
> > + int retry = I3C_TTY_RETRY;
> > + unsigned long flags;
> > + int actual;
> > + int ret;
> > +
> > + while (cnt > 0 && retry) {
> > + xfers.rnw = 0;
> > + xfers.len = CIRC_CNT_TO_END(circ->head, circ->tail, UART_XMIT_SIZE);
> > + xfers.len = min_t(u16, 16, xfers.len);
> > + xfers.data.out = circ->buf + circ->tail;
> > +
> > + ret = i3c_device_do_priv_xfers(sport->i3cdev, &xfers, 1);
> > + if (ret) {
> > + /*
> > + * Target side may not move data out of FIFO. delay can't resolve problem,
> > + * just reduce some possiblity. Target can't end I3C SDR mode write
> > + * transfer, discard data is reasonable when FIFO overrun.
> > + */
> > + usleep_range(I3C_TTY_YIELD_US, 10 * I3C_TTY_YIELD_US);
> > + retry--;
> > + } else {
> > + retry = 0;
> > + }
> > +
> > + actual = xfers.len;
> > +
> > + circ->tail = (circ->tail + actual) & (UART_XMIT_SIZE - 1);
> > +
> > + if (CIRC_CNT(circ->head, circ->tail, UART_XMIT_SIZE) < WAKEUP_CHARS)
> > + tty_port_tty_wakeup(&sport->port);
> > +
> > + cnt = CIRC_CNT(circ->head, circ->tail, UART_XMIT_SIZE);
> > + }
> > +
> > + spin_lock_irqsave(&sport->xlock, flags);
> > + if (circ->head == circ->tail)
> > + complete(&sport->txcomplete);
> > + spin_unlock_irqrestore(&sport->xlock, flags);
> > +}
> > +
> > +static int i3c_probe(struct i3c_device *i3cdev)
> > +{
> > + struct ttyi3c_port *port;
> > + struct device *tty_dev;
> > + struct i3c_ibi_setup req;
> > + int minor;
> > + int ret;
> > +
> > + port = devm_kzalloc(&i3cdev->dev, sizeof(*port), GFP_KERNEL);
> > + if (!port)
> > + return -ENOMEM;
> > +
> > + port->i3cdev = i3cdev;
> > + port->buffer = devm_kzalloc(&i3cdev->dev, UART_XMIT_SIZE, GFP_KERNEL);
> > + if (!port->buffer)
> > + return -ENOMEM;
> > +
> > + port->xmit.buf = devm_kzalloc(&i3cdev->dev, UART_XMIT_SIZE, GFP_KERNEL);
> > + if (!port->xmit.buf)
> > + return -ENOMEM;
>
> You allocate two pages even if you never use the device?
>
> > + dev_set_drvdata(&i3cdev->dev, port);
> > +
> > + req.max_payload_len = 8;
> > + req.num_slots = 4;
> > + req.handler = &i3c_controller_irq_handler;
> > +
> > + ret = i3c_device_request_ibi(i3cdev, &req);
> > + if (ret)
> > + return -EINVAL;
> > +
> > + mutex_lock(&i3c_tty_minors_lock);
> > + minor = idr_alloc(&i3c_tty_minors, port, 0, I3C_TTY_MINORS, GFP_KERNEL);
> > + mutex_unlock(&i3c_tty_minors_lock);
> > +
> > + if (minor < 0)
> > + return -EINVAL;
> > +
> > + spin_lock_init(&port->xlock);
> > + INIT_WORK(&port->txwork, tty_i3c_txwork);
> > + INIT_WORK(&port->rxwork, tty_i3c_rxwork);
> > + init_completion(&port->txcomplete);
> > +
> > + port->workqueue = alloc_workqueue("%s", 0, 0, dev_name(&i3cdev->dev));
> > + if (!port->workqueue)
> > + return -ENOMEM;
> > +
> > + tty_port_init(&port->port);
> > + port->port.ops = &i3c_port_ops;
> > +
> > + tty_dev = tty_port_register_device(&port->port, i3c_tty_driver, minor,
> > + &i3cdev->dev);
> > + if (IS_ERR(tty_dev)) {
> > + destroy_workqueue(port->workqueue);
>
> What about tty_port_put() (it drops the idr too)? And free ibi?
>
> > + return PTR_ERR(tty_dev);
> > + }
> > +
> > + port->minor = minor;
> > +
> > + return 0;
> > +}
> > +
> > +void i3c_remove(struct i3c_device *dev)
> > +{
> > + struct ttyi3c_port *sport = dev_get_drvdata(&dev->dev);
> > +
> > + tty_port_unregister_device(&sport->port, i3c_tty_driver, sport->minor);
> > + cancel_work_sync(&sport->txwork);
> > + destroy_workqueue(sport->workqueue);
>
> The same as above.
>
> > +}
> > +
> > +static struct i3c_driver i3c_driver = {
> > + .driver = {
> > + .name = "ttyi3c",
> > + },
> > + .probe = i3c_probe,
> > + .remove = i3c_remove,
> > + .id_table = i3c_ids,
> > +};
> > +
> > +static int __init i3c_tty_init(void)
> > +{
> > + int ret;
> > +
> > + i3c_tty_driver = tty_alloc_driver(I3C_TTY_MINORS,
> > + TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV);
> > +
> > + if (IS_ERR(i3c_tty_driver))
> > + return PTR_ERR(i3c_tty_driver);
> > +
> > + i3c_tty_driver->driver_name = "ttyI3C";
> > + i3c_tty_driver->name = "ttyI3C";
> > + i3c_tty_driver->minor_start = 0,
> > + i3c_tty_driver->type = TTY_DRIVER_TYPE_SERIAL,
> > + i3c_tty_driver->subtype = SERIAL_TYPE_NORMAL,
> > + i3c_tty_driver->init_termios = tty_std_termios;
> > + i3c_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL |
> > + CLOCAL;
> > + i3c_tty_driver->init_termios.c_lflag = 0;
> > +
> > + tty_set_operations(i3c_tty_driver, &i3c_tty_ops);
> > +
> > + ret = tty_register_driver(i3c_tty_driver);
> > + if (ret) {
> > + tty_driver_kref_put(i3c_tty_driver);
> > + return ret;
> > + }
> > +
> > + ret = i3c_driver_register(&i3c_driver);
> > + if (ret) {
> > + tty_unregister_driver(i3c_tty_driver);
> > + tty_driver_kref_put(i3c_tty_driver);
>
> Use goto + fail paths. And in i3c_probe() too.
>
> > + }
> > +
> > + return ret;
> > +}
>
>
> regards,
> --
> js
> suse labs
>
On Wed, Oct 18, 2023 at 05:11:09PM -0400, Frank Li wrote:
> Add new driver to allow tty over i3c master.
As Jiri said, you need a lot more description here before we can even
start to review it.
Please fix up and send a v2.
thanks,
greg k-h
On Thu, Oct 19, 2023 at 09:12:58AM +0200, Jiri Slaby wrote:
> On 18. 10. 23, 23:11, Frank Li wrote:
> > Add new driver to allow tty over i3c master.
>
> What is it good for? Why we should consider this for inclusion at all?
>
> > Signed-off-by: Frank Li <[email protected]>
> > ---
> >
> > Notes:
> > This patch depend on
> > https://lore.kernel.org/imx/[email protected]/T/#t
> >
> > drivers/tty/Kconfig | 8 +
> > drivers/tty/Makefile | 1 +
> > drivers/tty/i3c_tty.c | 466 ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 475 insertions(+)
> > create mode 100644 drivers/tty/i3c_tty.c
> >
> > diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> > index 5646dc6242cd9..6d91fe6a211a1 100644
> > --- a/drivers/tty/Kconfig
> > +++ b/drivers/tty/Kconfig
> > @@ -412,6 +412,14 @@ config RPMSG_TTY
> > To compile this driver as a module, choose M here: the module will be
> > called rpmsg_tty.
> > +config I3C_TTY
> > + tristate "tty over i3c"
>
> "TTY over I3C"
>
> > + depends on I3C
> > + help
> > + Select this options if you'd like use tty over I3C master controller
>
> TTY and add a period.
>
> > --- /dev/null
> > +++ b/drivers/tty/i3c_tty.c
> > @@ -0,0 +1,466 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2023 NXP.
> > + *
> > + * Author: Frank Li <[email protected]>
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/i3c/device.h>
> > +#include <linux/i3c/master.h>
> > +#include <linux/slab.h>
> > +#include <linux/console.h>
> > +#include <linux/serial_core.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/tty_flip.h>
> > +
> > +static DEFINE_IDR(i3c_tty_minors);
> > +static DEFINE_MUTEX(i3c_tty_minors_lock);
> > +
> > +static struct tty_driver *i3c_tty_driver;
> > +
> > +#define I3C_TTY_MINORS 256
> > +#define I3C_TTY_TRANS_SIZE 16
> > +#define I3C_TTY_RX_STOP BIT(0)
> > +#define I3C_TTY_RETRY 20
> > +#define I3C_TTY_YIELD_US 100
> > +
> > +struct ttyi3c_port {
> > + struct tty_port port;
> > + int minor;
> > + unsigned int txfifo_size;
> > + unsigned int rxfifo_size;
> > + struct circ_buf xmit;
>
> Why can't you use port.xmit_fifo?
>
> > + spinlock_t xlock; /* protect xmit */
> > + void *buffer;
> > + struct i3c_device *i3cdev;
> > + struct work_struct txwork;
> > + struct work_struct rxwork;
> > + struct completion txcomplete;
> > + struct workqueue_struct *workqueue;
>
> Why do you need a special wq? And even, why per port?
>
> > + atomic_t status;
> > +};
> > +
> > +static const struct i3c_device_id i3c_ids[] = {
> > + I3C_DEVICE(0x011B, 0x1000, NULL),
> > + { /* sentinel */ },
> > +};
> > +
> > +static int i3c_port_activate(struct tty_port *port, struct tty_struct *tty)
> > +{
> > + struct ttyi3c_port *sport = container_of(port, struct ttyi3c_port, port);
> > +
> > + atomic_set(&sport->status, 0);
> > +
> > + return i3c_device_enable_ibi(sport->i3cdev);
> > +}
> > +
> > +static void i3c_port_shutdown(struct tty_port *port)
> > +{
> > + struct ttyi3c_port *sport =
> > + container_of(port, struct ttyi3c_port, port);
> > +
> > + i3c_device_disable_ibi(sport->i3cdev);
> > +}
> > +
> > +static void i3c_port_destruct(struct tty_port *port)
> > +{
> > + struct ttyi3c_port *sport =
> > + container_of(port, struct ttyi3c_port, port);
> > +
> > + mutex_lock(&i3c_tty_minors_lock);
> > + idr_remove(&i3c_tty_minors, sport->minor);
> > + mutex_unlock(&i3c_tty_minors_lock);
> > +}
> > +
> > +static const struct tty_port_operations i3c_port_ops = {
> > + .shutdown = i3c_port_shutdown,
> > + .activate = i3c_port_activate,
> > + .destruct = i3c_port_destruct,
> > +};
> > +
> > +static struct ttyi3c_port *i3c_get_by_minor(unsigned int minor)
> > +{
> > + struct ttyi3c_port *sport;
> > +
> > + mutex_lock(&i3c_tty_minors_lock);
> > + sport = idr_find(&i3c_tty_minors, minor);
> > + mutex_unlock(&i3c_tty_minors_lock);
> > +
> > + return sport;
> > +}
> > +
> > +static int i3c_install(struct tty_driver *driver, struct tty_struct *tty)
> > +{
> > + struct ttyi3c_port *sport;
> > + int ret;
> > +
> > + sport = i3c_get_by_minor(tty->index);
> > + if (!sport)
> > + return -ENODEV;
> > +
> > + ret = tty_standard_install(driver, tty);
> > + if (ret)
> > + return ret;
> > +
> > + tty->driver_data = sport;
> > +
> > + return 0;
> > +}
>
> You don't need this at all. (Watch for XXX marks below.)
>
> > +static ssize_t i3c_write(struct tty_struct *tty, const unsigned char *buf, size_t count)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > + struct circ_buf *circ = &sport->xmit;
> > + unsigned long flags;
> > + int c, ret = 0;
> > +
> > + spin_lock_irqsave(&sport->xlock, flags);
> > + while (1) {
> > + c = CIRC_SPACE_TO_END(circ->head, circ->tail, UART_XMIT_SIZE);
> > + if (count < c)
> > + c = count;
> > + if (c <= 0)
> > + break;
> > +
> > + memcpy(circ->buf + circ->head, buf, c);
> > + circ->head = (circ->head + c) & (UART_XMIT_SIZE - 1);
> > + buf += c;
> > + count -= c;
> > + ret += c;
> > + }
> > + spin_unlock_irqrestore(&sport->xlock, flags);
>
> With kfifo, all this would be one line, right?
>
> > +
> > + if (circ->head != circ->tail)
> > + queue_work(sport->workqueue, &sport->txwork);
> > +
> > + return ret;
> > +}
> > +
> > +static int i3c_put_char(struct tty_struct *tty, unsigned char ch)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > + struct circ_buf *circ = &sport->xmit;
> > + unsigned long flags;
> > + int ret = 0;
> > +
> > + spin_lock_irqsave(&sport->xlock, flags);
> > +
> > + if (sport && CIRC_SPACE(circ->head, circ->tail, UART_XMIT_SIZE) != 0) {
> > + circ->buf[circ->head] = ch;
> > + circ->head = (circ->head + 1) & (UART_XMIT_SIZE - 1);
> > + ret = 1;
> > + }
> > +
> > + spin_unlock_irqrestore(&sport->xlock, flags);
> > +
> > + return ret;
> > +}
> > +
> > +static void i3c_flush_chars(struct tty_struct *tty)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > +
> > + queue_work(sport->workqueue, &sport->txwork);
> > +}
> > +
> > +static unsigned int i3c_write_room(struct tty_struct *tty)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > + struct circ_buf *circ = &sport->xmit;
> > + unsigned long flags;
> > + int ret = 0;
> > +
> > + spin_lock_irqsave(&sport->xlock, flags);
> > + ret = CIRC_SPACE(circ->head, circ->tail, UART_XMIT_SIZE);
> > + spin_unlock_irqrestore(&sport->xlock, flags);
> > +
> > + return ret;
> > +}
> > +
> > +static void i3c_throttle(struct tty_struct *tty)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > +
> > + atomic_or(I3C_TTY_RX_STOP, &sport->status);
> > +}
> > +
> > +static void i3c_unthrottle(struct tty_struct *tty)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > +
> > + atomic_andnot(I3C_TTY_RX_STOP, &sport->status);
> > +
> > + queue_work(sport->workqueue, &sport->rxwork);
> > +}
> > +
> > +static int i3c_open(struct tty_struct *tty, struct file *filp)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
>
> XXX Here, you can simply do:
>
> struct ttyi3c_port *sport = container_of(tty->port, struct ttyi3c_port,
> port);
>
> tty->driver_data = sport;
>
>
> > + return tty_port_open(&sport->port, tty, filp);
> > +}
> > +
> > +static void i3c_close(struct tty_struct *tty, struct file *filp)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > +
> > + if (!sport)
> > + return;
>
> How can that happen?
>
> > + tty_port_close(tty->port, tty, filp);
> > +}
> > +
> > +static void i3c_wait_until_sent(struct tty_struct *tty, int timeout)
> > +{
> > + struct ttyi3c_port *sport = tty->driver_data;
> > +
> > + wait_for_completion_timeout(&sport->txcomplete, timeout);
> > + reinit_completion(&sport->txcomplete);
>
> Does this work in parallel?
>
> > +}
> > +
> > +static const struct tty_operations i3c_tty_ops = {
> > + .install = i3c_install,
> > + .open = i3c_open,
> > + .close = i3c_close,
> > + .write = i3c_write,
> > + .put_char = i3c_put_char,
> > + .flush_chars = i3c_flush_chars,
> > + .write_room = i3c_write_room,
> > + .throttle = i3c_throttle,
> > + .unthrottle = i3c_unthrottle,
> > + .wait_until_sent = i3c_wait_until_sent,
> > +};
> > +
> > +static void i3c_controller_irq_handler(struct i3c_device *dev,
> > + const struct i3c_ibi_payload *payload)
> > +{
> > + struct ttyi3c_port *sport = dev_get_drvdata(&dev->dev);
> > +
> > + queue_work(sport->workqueue, &sport->rxwork);
>
> Doesn't ibi provide threaded irqs? Oh, wait, i3c_master_handle_ibi() is
> already a work!
rxwork need be trigger by two sources: one from IBI irq, another is from
i3c_unthrottle().
If unthrottle just clear flags and IBI irq already missed, so rx will stop
work util new IBI coming.
I hope rxwork can continue to fetch left data. i3c_tty driver can't issue
a fake IBI here.
So using sperate rxwork. both IBI and unthrottle to trigger such work, make
rx can get all data from slave side.
Frank
>
> > +}
> > +
> > +static void tty_i3c_rxwork(struct work_struct *work)
> > +{
> > + struct ttyi3c_port *sport = container_of(work, struct ttyi3c_port, rxwork);
> > + struct i3c_priv_xfer xfers;
> > + int retry = I3C_TTY_RETRY;
> > + u16 status = BIT(0);
> > +
> > + do {
> > + memset(&xfers, 0, sizeof(xfers));
> > + xfers.data.in = sport->buffer;
> > + xfers.len = I3C_TTY_TRANS_SIZE;
> > + xfers.rnw = 1;
> > +
> > + if (I3C_TTY_RX_STOP & atomic_read(&sport->status))
>
> Hmm, why not much simpler (and yet atomic) {set,test,clear}_bit()?
>
> > + break;
> > +
> > + i3c_device_do_priv_xfers(sport->i3cdev, &xfers, 1);
> > +
> > + if (xfers.actual_len) {
> > + tty_insert_flip_string(&sport->port, sport->buffer, xfers.actual_len);
>
> This can fail.
>
> > + retry = 20;
> > + continue;
> > + } else {
>
> "} else {" uneeded.
>
> > + status = BIT(0);
> > + i3c_device_getstatus_format1(sport->i3cdev, &status);
> > + /*
> > + * Target side need some time to fill data into fifo. Target side may not
>
> "needs"
>
> > + * have hardware update status in real time. Software update status always
> > + * need some delays.
>
> "needs"
>
> > + *
> > + * Generally, target side have cicular buffer in memory, it will be moved
> "circular" all around.
>
> > + * into FIFO by CPU or DMA. 'status' just show if cicular buffer empty. But
> > + * there are gap, espcially CPU have not response irq to fill FIFO in time.
>
> espcially
>
> > + * So xfers.actual will be zero, wait for little time to avoid flood
> > + * transfer in i3c bus.
> > + */
> > + usleep_range(I3C_TTY_YIELD_US, 10 * I3C_TTY_YIELD_US);
> > + retry--;
> > + }
> > +
> > + } while (retry && (status & BIT(0)));
> > +
> > + tty_flip_buffer_push(&sport->port);
> > +}
> > +
> > +static void tty_i3c_txwork(struct work_struct *work)
> > +{
> > + struct ttyi3c_port *sport = container_of(work, struct ttyi3c_port, txwork);
> > + struct circ_buf *circ = &sport->xmit;
> > + int cnt = CIRC_CNT(circ->head, circ->tail, UART_XMIT_SIZE);
> > + struct i3c_priv_xfer xfers;
> > + int retry = I3C_TTY_RETRY;
> > + unsigned long flags;
> > + int actual;
> > + int ret;
> > +
> > + while (cnt > 0 && retry) {
> > + xfers.rnw = 0;
> > + xfers.len = CIRC_CNT_TO_END(circ->head, circ->tail, UART_XMIT_SIZE);
> > + xfers.len = min_t(u16, 16, xfers.len);
> > + xfers.data.out = circ->buf + circ->tail;
> > +
> > + ret = i3c_device_do_priv_xfers(sport->i3cdev, &xfers, 1);
> > + if (ret) {
> > + /*
> > + * Target side may not move data out of FIFO. delay can't resolve problem,
> > + * just reduce some possiblity. Target can't end I3C SDR mode write
> > + * transfer, discard data is reasonable when FIFO overrun.
> > + */
> > + usleep_range(I3C_TTY_YIELD_US, 10 * I3C_TTY_YIELD_US);
> > + retry--;
> > + } else {
> > + retry = 0;
> > + }
> > +
> > + actual = xfers.len;
> > +
> > + circ->tail = (circ->tail + actual) & (UART_XMIT_SIZE - 1);
> > +
> > + if (CIRC_CNT(circ->head, circ->tail, UART_XMIT_SIZE) < WAKEUP_CHARS)
> > + tty_port_tty_wakeup(&sport->port);
> > +
> > + cnt = CIRC_CNT(circ->head, circ->tail, UART_XMIT_SIZE);
> > + }
> > +
> > + spin_lock_irqsave(&sport->xlock, flags);
> > + if (circ->head == circ->tail)
> > + complete(&sport->txcomplete);
> > + spin_unlock_irqrestore(&sport->xlock, flags);
> > +}
> > +
> > +static int i3c_probe(struct i3c_device *i3cdev)
> > +{
> > + struct ttyi3c_port *port;
> > + struct device *tty_dev;
> > + struct i3c_ibi_setup req;
> > + int minor;
> > + int ret;
> > +
> > + port = devm_kzalloc(&i3cdev->dev, sizeof(*port), GFP_KERNEL);
> > + if (!port)
> > + return -ENOMEM;
> > +
> > + port->i3cdev = i3cdev;
> > + port->buffer = devm_kzalloc(&i3cdev->dev, UART_XMIT_SIZE, GFP_KERNEL);
> > + if (!port->buffer)
> > + return -ENOMEM;
> > +
> > + port->xmit.buf = devm_kzalloc(&i3cdev->dev, UART_XMIT_SIZE, GFP_KERNEL);
> > + if (!port->xmit.buf)
> > + return -ENOMEM;
>
> You allocate two pages even if you never use the device?
>
> > + dev_set_drvdata(&i3cdev->dev, port);
> > +
> > + req.max_payload_len = 8;
> > + req.num_slots = 4;
> > + req.handler = &i3c_controller_irq_handler;
> > +
> > + ret = i3c_device_request_ibi(i3cdev, &req);
> > + if (ret)
> > + return -EINVAL;
> > +
> > + mutex_lock(&i3c_tty_minors_lock);
> > + minor = idr_alloc(&i3c_tty_minors, port, 0, I3C_TTY_MINORS, GFP_KERNEL);
> > + mutex_unlock(&i3c_tty_minors_lock);
> > +
> > + if (minor < 0)
> > + return -EINVAL;
> > +
> > + spin_lock_init(&port->xlock);
> > + INIT_WORK(&port->txwork, tty_i3c_txwork);
> > + INIT_WORK(&port->rxwork, tty_i3c_rxwork);
> > + init_completion(&port->txcomplete);
> > +
> > + port->workqueue = alloc_workqueue("%s", 0, 0, dev_name(&i3cdev->dev));
> > + if (!port->workqueue)
> > + return -ENOMEM;
> > +
> > + tty_port_init(&port->port);
> > + port->port.ops = &i3c_port_ops;
> > +
> > + tty_dev = tty_port_register_device(&port->port, i3c_tty_driver, minor,
> > + &i3cdev->dev);
> > + if (IS_ERR(tty_dev)) {
> > + destroy_workqueue(port->workqueue);
>
> What about tty_port_put() (it drops the idr too)? And free ibi?
>
> > + return PTR_ERR(tty_dev);
> > + }
> > +
> > + port->minor = minor;
> > +
> > + return 0;
> > +}
> > +
> > +void i3c_remove(struct i3c_device *dev)
> > +{
> > + struct ttyi3c_port *sport = dev_get_drvdata(&dev->dev);
> > +
> > + tty_port_unregister_device(&sport->port, i3c_tty_driver, sport->minor);
> > + cancel_work_sync(&sport->txwork);
> > + destroy_workqueue(sport->workqueue);
>
> The same as above.
>
> > +}
> > +
> > +static struct i3c_driver i3c_driver = {
> > + .driver = {
> > + .name = "ttyi3c",
> > + },
> > + .probe = i3c_probe,
> > + .remove = i3c_remove,
> > + .id_table = i3c_ids,
> > +};
> > +
> > +static int __init i3c_tty_init(void)
> > +{
> > + int ret;
> > +
> > + i3c_tty_driver = tty_alloc_driver(I3C_TTY_MINORS,
> > + TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV);
> > +
> > + if (IS_ERR(i3c_tty_driver))
> > + return PTR_ERR(i3c_tty_driver);
> > +
> > + i3c_tty_driver->driver_name = "ttyI3C";
> > + i3c_tty_driver->name = "ttyI3C";
> > + i3c_tty_driver->minor_start = 0,
> > + i3c_tty_driver->type = TTY_DRIVER_TYPE_SERIAL,
> > + i3c_tty_driver->subtype = SERIAL_TYPE_NORMAL,
> > + i3c_tty_driver->init_termios = tty_std_termios;
> > + i3c_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL |
> > + CLOCAL;
> > + i3c_tty_driver->init_termios.c_lflag = 0;
> > +
> > + tty_set_operations(i3c_tty_driver, &i3c_tty_ops);
> > +
> > + ret = tty_register_driver(i3c_tty_driver);
> > + if (ret) {
> > + tty_driver_kref_put(i3c_tty_driver);
> > + return ret;
> > + }
> > +
> > + ret = i3c_driver_register(&i3c_driver);
> > + if (ret) {
> > + tty_unregister_driver(i3c_tty_driver);
> > + tty_driver_kref_put(i3c_tty_driver);
>
> Use goto + fail paths. And in i3c_probe() too.
>
> > + }
> > +
> > + return ret;
> > +}
>
>
> regards,
> --
> js
> suse labs
>
Hi Frank,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on tty/tty-next tty/tty-linus linus/master v6.6-rc6 next-20231019]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Frank-Li/tty-i3c-add-tty-over-i3c-master-support/20231019-051407
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link: https://lore.kernel.org/r/20231018211111.3437929-1-Frank.Li%40nxp.com
patch subject: [PATCH 1/1] tty: i3c: add tty over i3c master support
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20231020/[email protected]/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231020/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/tty/i3c_tty.c: In function 'tty_i3c_rxwork':
drivers/tty/i3c_tty.c:265:26: error: 'struct i3c_priv_xfer' has no member named 'actual_len'
265 | if (xfers.actual_len) {
| ^
drivers/tty/i3c_tty.c:266:82: error: 'struct i3c_priv_xfer' has no member named 'actual_len'
266 | tty_insert_flip_string(&sport->port, sport->buffer, xfers.actual_len);
| ^
drivers/tty/i3c_tty.c:271:25: error: implicit declaration of function 'i3c_device_getstatus_format1' [-Werror=implicit-function-declaration]
271 | i3c_device_getstatus_format1(sport->i3cdev, &status);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/tty/i3c_tty.c: At top level:
>> drivers/tty/i3c_tty.c:400:6: warning: no previous prototype for 'i3c_remove' [-Wmissing-prototypes]
400 | void i3c_remove(struct i3c_device *dev)
| ^~~~~~~~~~
cc1: some warnings being treated as errors
vim +/i3c_remove +400 drivers/tty/i3c_tty.c
399
> 400 void i3c_remove(struct i3c_device *dev)
401 {
402 struct ttyi3c_port *sport = dev_get_drvdata(&dev->dev);
403
404 tty_port_unregister_device(&sport->port, i3c_tty_driver, sport->minor);
405 cancel_work_sync(&sport->txwork);
406 destroy_workqueue(sport->workqueue);
407 }
408
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Frank,
kernel test robot noticed the following build errors:
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on tty/tty-next tty/tty-linus linus/master v6.6 next-20231106]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Frank-Li/tty-i3c-add-tty-over-i3c-master-support/20231019-051407
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link: https://lore.kernel.org/r/20231018211111.3437929-1-Frank.Li%40nxp.com
patch subject: [PATCH 1/1] tty: i3c: add tty over i3c master support
config: microblaze-allyesconfig (https://download.01.org/0day-ci/archive/20231107/[email protected]/config)
compiler: microblaze-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231107/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
drivers/tty/i3c_tty.c: In function 'tty_i3c_rxwork':
>> drivers/tty/i3c_tty.c:265:26: error: 'struct i3c_priv_xfer' has no member named 'actual_len'
265 | if (xfers.actual_len) {
| ^
drivers/tty/i3c_tty.c:266:82: error: 'struct i3c_priv_xfer' has no member named 'actual_len'
266 | tty_insert_flip_string(&sport->port, sport->buffer, xfers.actual_len);
| ^
>> drivers/tty/i3c_tty.c:271:25: error: implicit declaration of function 'i3c_device_getstatus_format1' [-Werror=implicit-function-declaration]
271 | i3c_device_getstatus_format1(sport->i3cdev, &status);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/tty/i3c_tty.c: At top level:
drivers/tty/i3c_tty.c:400:6: warning: no previous prototype for 'i3c_remove' [-Wmissing-prototypes]
400 | void i3c_remove(struct i3c_device *dev)
| ^~~~~~~~~~
cc1: some warnings being treated as errors
vim +265 drivers/tty/i3c_tty.c
246
247 static void tty_i3c_rxwork(struct work_struct *work)
248 {
249 struct ttyi3c_port *sport = container_of(work, struct ttyi3c_port, rxwork);
250 struct i3c_priv_xfer xfers;
251 int retry = I3C_TTY_RETRY;
252 u16 status = BIT(0);
253
254 do {
255 memset(&xfers, 0, sizeof(xfers));
256 xfers.data.in = sport->buffer;
257 xfers.len = I3C_TTY_TRANS_SIZE;
258 xfers.rnw = 1;
259
260 if (I3C_TTY_RX_STOP & atomic_read(&sport->status))
261 break;
262
263 i3c_device_do_priv_xfers(sport->i3cdev, &xfers, 1);
264
> 265 if (xfers.actual_len) {
266 tty_insert_flip_string(&sport->port, sport->buffer, xfers.actual_len);
267 retry = 20;
268 continue;
269 } else {
270 status = BIT(0);
> 271 i3c_device_getstatus_format1(sport->i3cdev, &status);
272 /*
273 * Target side need some time to fill data into fifo. Target side may not
274 * have hardware update status in real time. Software update status always
275 * need some delays.
276 *
277 * Generally, target side have cicular buffer in memory, it will be moved
278 * into FIFO by CPU or DMA. 'status' just show if cicular buffer empty. But
279 * there are gap, espcially CPU have not response irq to fill FIFO in time.
280 * So xfers.actual will be zero, wait for little time to avoid flood
281 * transfer in i3c bus.
282 */
283 usleep_range(I3C_TTY_YIELD_US, 10 * I3C_TTY_YIELD_US);
284 retry--;
285 }
286
287 } while (retry && (status & BIT(0)));
288
289 tty_flip_buffer_push(&sport->port);
290 }
291
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki