Re: [PATCH v4 8/8] tty: i3c: add TTY over I3C master support

[Ilpo Järvinen]

On Tue, 23 Jan 2024, Frank Li wrote:

> In typical embedded Linux systems, UART consoles require at least two pins,
> TX and RX. In scenarios where I2C/I3C devices like sensors or PMICs are
> present, we can save these two pins by using this driver. Pins is crucial
> resources, especially in small chip packages.
> 
> This introduces support for using the I3C bus to transfer console tty data,
> effectively replacing the need for dedicated UART pins. This not only
> conserves valuable pin resources but also facilitates testing of I3C's
> advanced features, including early termination, in-band interrupt (IBI)
> support, and the creation of more complex data patterns. Additionally,
> it aids in identifying and addressing issues within the I3C controller
> driver.
> 
> Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> ---
> 
> Notes:
>     Notes:
>         Version number use i3c target patches.
>         Change from v3 to v4
>         - add static at i3c_remove()
>         Change v2
>         - using system_unbound_wq working queue
>         - fixed accoring to Jiri Slaby's comments
>     
>         Change before send with i3c target support
>     
>         Change from v4 to v5
>         - send in i3c improvememtn patches.
>     
>         Change from v2 to v4
>         - none
>     
>         Change from v1 to v2
>         - update commit message.
>         - using goto for err handle
>         - using one working queue for all tty-i3c device
>         - fixed typo found by js
>         - update kconfig help
>         - using kfifo
>     
>         Still below items not be fixed (according to Jiri Slaby's comments)
>         - rxwork thread: need trigger from two position.
>         - common thread queue: need some suggestion
> 
>  drivers/tty/Kconfig   |  13 ++
>  drivers/tty/Makefile  |   1 +
>  drivers/tty/i3c_tty.c | 426 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 440 insertions(+)
>  create mode 100644 drivers/tty/i3c_tty.c
> 
> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index 5646dc6242cd9..9ab4cd480e9f8 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -412,6 +412,19 @@ 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 option to use TTY over I3C master controller.
> +
> +	  This makes it possible for user-space programs to send and receive
> +	  data as a standard tty protocol. I3C provide relatively higher data
> +	  transfer rate and less pin numbers, SDA/SCL are shared with other
> +	  devices.
> +
> +	  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..8f4e87dfa01cd
> --- /dev/null
> +++ b/drivers/tty/i3c_tty.c
> @@ -0,0 +1,426 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2023 NXP.
> + *
> + * Author: Frank Li <Frank.Li@xxxxxxx>
> + */
> +
> +#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>

Do you need this header.

> +#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		8
> +#define I3C_TTY_TRANS_SIZE	16
> +#define I3C_TTY_RX_STOP		0
> +#define I3C_TTY_RETRY		20
> +#define I3C_TTY_YIELD_US	100
> +
> +struct ttyi3c_port {
> +	struct tty_port port;

Missing #include

> +	int minor;
> +	spinlock_t xlock; /* protect xmit */

Missing #include

> +	u8 tx_buff[I3C_TTY_TRANS_SIZE];
> +	u8 rx_buff[I3C_TTY_TRANS_SIZE];
> +	struct i3c_device *i3cdev;
> +	struct work_struct txwork;
> +	struct work_struct rxwork;
> +	struct completion txcomplete;

Missing #include

> +	unsigned long status;
> +	u32 buf_overrun;
> +};

> +static void i3c_port_shutdown(struct tty_port *port)
> +{
> +	struct ttyi3c_port *sport =
> +		container_of(port, struct ttyi3c_port, port);

On one line.

> +
> +	i3c_device_disable_ibi(sport->i3cdev);
> +	tty_port_free_xmit_buf(port);
> +}
> +
> +static void i3c_port_destruct(struct tty_port *port)
> +{
> +	struct ttyi3c_port *sport =
> +		container_of(port, struct ttyi3c_port, port);

Ditto.

> +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;
> +	u32 retry = I3C_TTY_RETRY;
> +	u16 status = BIT(0);

Unnecessary initialization.

> +	int ret;
> +
> +	memset(&xfers, 0, sizeof(xfers));
> +	xfers.data.in = sport->rx_buff;
> +	xfers.len = I3C_TTY_TRANS_SIZE;
> +	xfers.rnw = 1;
> +
> +	do {
> +		if (test_bit(I3C_TTY_RX_STOP, &sport->status))
> +			break;
> +
> +		i3c_device_do_priv_xfers(sport->i3cdev, &xfers, 1);
> +
> +		if (xfers.actual_len) {
> +			ret = tty_insert_flip_string(&sport->port, sport->rx_buff,
> +						     xfers.actual_len);
> +			if (ret < xfers.actual_len)
> +				sport->buf_overrun++;
> +
> +			retry = I3C_TTY_RETRY;
> +			continue;
> +		}
> +
> +		status = BIT(0);

Can this BIT(0) be named with a #define?

> +		i3c_device_getstatus_format1(sport->i3cdev, &status);
> +		/*
> +		 * Target side needs some time to fill data into fifo. Target side may not
> +		 * have hardware update status in real time. Software update status always
> +		 * needs some delays.
> +		 *
> +		 * Generally, target side have circular buffer in memory, it will be moved
> +		 * into FIFO by CPU or DMA. 'status' just show if circular buffer empty. But
> +		 * there are gap, especially 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)));

Name with define?


-- 
 i.