diff mbox series

[1/1] tty: i3c: add tty over i3c master support

Message ID 20231018211111.3437929-1-Frank.Li@nxp.com (mailing list archive)
State Superseded
Headers show
Series [1/1] tty: i3c: add tty over i3c master support | expand

Commit Message

Frank Li Oct. 18, 2023, 9:11 p.m. UTC
Add new driver to allow tty over i3c master.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---

Notes:
    This patch depend on
    https://lore.kernel.org/imx/20231018205929.3435110-1-Frank.Li@nxp.com/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

Comments

Jiri Slaby Oct. 19, 2023, 7:12 a.m. UTC | #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 <Frank.Li@nxp.com>
> ---
> 
> Notes:
>      This patch depend on
>      https://lore.kernel.org/imx/20231018205929.3435110-1-Frank.Li@nxp.com/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 <Frank.Li@nxp.com>
> + */
> +
> +#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,
Frank Li Oct. 19, 2023, 2:49 p.m. UTC | #2
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 <Frank.Li@nxp.com>
> > ---
> > 
> > Notes:
> >      This patch depend on
> >      https://lore.kernel.org/imx/20231018205929.3435110-1-Frank.Li@nxp.com/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 <Frank.Li@nxp.com>
> > + */
> > +
> > +#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
>
Greg Kroah-Hartman Oct. 19, 2023, 3:38 p.m. UTC | #3
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
Frank Li Oct. 19, 2023, 8:21 p.m. UTC | #4
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 <Frank.Li@nxp.com>
> > ---
> > 
> > Notes:
> >      This patch depend on
> >      https://lore.kernel.org/imx/20231018205929.3435110-1-Frank.Li@nxp.com/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 <Frank.Li@nxp.com>
> > + */
> > +
> > +#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
>
kernel test robot Oct. 20, 2023, 12:02 p.m. UTC | #5
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/202310201933.9lZn2Ebl-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231020/202310201933.9lZn2Ebl-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310201933.9lZn2Ebl-lkp@intel.com/

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
kernel test robot Nov. 6, 2023, 8:22 p.m. UTC | #6
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/202311070330.5mylauLR-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231107/202311070330.5mylauLR-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311070330.5mylauLR-lkp@intel.com/

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
diff mbox series

Patch

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 <Frank.Li@nxp.com>
+ */
+
+#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");