[PATCH v7 0/1] usb: serial: add support for CH348

[PATCH v7 0/1] usb: serial: add support for CH348

[Corentin Labbe]

Hello

The CH348 is an octo serial to USB adapter.
The following patch adds a driver for supporting it.
Since there is no public datasheet, unfortunatly it remains some magic values.

It was tested with a large range of baud from 1200 to 1500000 and used with
success in one of our kernel CI testlab.

Regards

Changes since v1:
- use a data structure for encoding/decoding messages.
- check if needed endpoints exists
- fix URB leak in ch348_allocate_status_read error case
- test for maximum baud rate as stated by datasheet

Changes since v2:
- specify ch348_rxbuf data length
- Use correct speed_t dwDTERate instead of __le32
- test for maximum baud rate supported according to datasheet
- Use a define for CH348_TX_HDRSIZE

Changes since v3
- Fixed all reported problem from https://lore.kernel.org/lkml/Y5NDwEakGJbmB6+b@Red/T/#mb6234d0427cfdabf412190565e215995a41482dd
  Mostly reworked the endpoint mux to be the same than mx_uport

Changes since v4:
- The V4 was sent against stable and next have ch348_set_termios ktermios
  parameter const that I forgot to change

Changes since v5:
- Fixed all reported problem from https://lore.kernel.org/lkml/20230106135338.643951-1-clabbe@baylibre.com/T/#m044aab24dfb652ea34aa06f8ef704da9d6a2e036
- Major change is dropping of all status handling which was unused.
  It will be probably necessary to bring it back when using GPIO.
  This will be done when I will finish my next devboard.

Changes since v6:
- read and print the device version during probe
- Only request one bulk out channel from usb-serial core
- Implement status report / interrupt handling
- Fix buffer->rate calculation / enable support for slow baud rates
- use a mutex to protect against concurrent writes
- split write buffers for slow baud rates

Important note, v7 is mostly done from work of Martin Blumenstingl,
so the changelog was built from https://github.com/xdarklight/ch348/commits/main/
Great thanks to him

Corentin Labbe (1):
  usb: serial: add support for CH348

 drivers/usb/serial/Kconfig  |   9 +
 drivers/usb/serial/Makefile |   1 +
 drivers/usb/serial/ch348.c  | 725 ++++++++++++++++++++++++++++++++++++
 3 files changed, 735 insertions(+)
 create mode 100644 drivers/usb/serial/ch348.c

-- 
2.43.2

[PATCH 1/1 v7] usb: serial: add support for CH348

[Corentin Labbe]

The CH348 is an USB octo port serial adapter.
The device multiplexes all 8 ports in the same pair of Bulk endpoints.
Since there is no public datasheet, unfortunately it remains some magic values

Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
---
 drivers/usb/serial/Kconfig  |   9 +
 drivers/usb/serial/Makefile |   1 +
 drivers/usb/serial/ch348.c  | 725 ++++++++++++++++++++++++++++++++++++
 3 files changed, 735 insertions(+)
 create mode 100644 drivers/usb/serial/ch348.c

diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index ef8d1c73c754..1e1842656b54 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -112,6 +112,15 @@ config USB_SERIAL_CH341
 	  To compile this driver as a module, choose M here: the
 	  module will be called ch341.
 
+config USB_SERIAL_CH348
+	tristate "USB Winchiphead CH348 Octo Port Serial Driver"
+	help
+	  Say Y here if you want to use a Winchiphead CH348 octo port
+	  USB to serial adapter.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ch348.
+
 config USB_SERIAL_WHITEHEAT
 	tristate "USB ConnectTech WhiteHEAT Serial Driver"
 	select USB_EZUSB_FX2
diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
index c7bb1a88173e..d16ff59fde68 100644
--- a/drivers/usb/serial/Makefile
+++ b/drivers/usb/serial/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_USB_SERIAL_AIRCABLE)		+= aircable.o
 obj-$(CONFIG_USB_SERIAL_ARK3116)		+= ark3116.o
 obj-$(CONFIG_USB_SERIAL_BELKIN)			+= belkin_sa.o
 obj-$(CONFIG_USB_SERIAL_CH341)			+= ch341.o
+obj-$(CONFIG_USB_SERIAL_CH348)			+= ch348.o
 obj-$(CONFIG_USB_SERIAL_CP210X)			+= cp210x.o
 obj-$(CONFIG_USB_SERIAL_CYBERJACK)		+= cyberjack.o
 obj-$(CONFIG_USB_SERIAL_CYPRESS_M8)		+= cypress_m8.o
diff --git a/drivers/usb/serial/ch348.c b/drivers/usb/serial/ch348.c
new file mode 100644
index 000000000000..e437ba36bb5e
--- /dev/null
+++ b/drivers/usb/serial/ch348.c
@@ -0,0 +1,725 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * USB serial driver for USB to Octal UARTs chip ch348.
+ *
+ * Copyright (C) 2022 Corentin Labbe <clabbe@baylibre.com>
+ * With the help of Neil Armstrong <neil.armstrong@linaro.org>
+ * and the help of Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+ */
+
+#include <linux/completion.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/overflow.h>
+#include <linux/serial.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/tty_driver.h>
+#include <linux/tty_flip.h>
+#include <linux/usb.h>
+#include <linux/usb/serial.h>
+
+#define CH348_CMD_TIMEOUT   2000
+
+#define CH348_CTO_D	0x01
+#define CH348_CTO_R	0x02
+
+#define CH348_CTI_C	0x10
+#define CH348_CTI_DSR	0x20
+#define CH348_CTI_R	0x40
+#define CH348_CTI_DCD	0x80
+
+#define CH348_LO	0x02
+#define CH348_LP	0x04
+#define CH348_LF	0x08
+#define CH348_LB	0x10
+
+#define CMD_W_R		0xC0
+#define CMD_W_BR	0x80
+
+#define CMD_WB_E	0x90
+#define CMD_RB_E	0xC0
+
+#define M_NOR		0x00
+#define M_HF		0x03
+
+#define R_MOD		0x97
+#define R_IO_D		0x98
+#define R_IO_O		0x99
+#define R_IO_I		0x9b
+#define R_TM_O		0x9c
+#define R_INIT		0xa1
+
+#define R_C1		0x01
+#define R_C2		0x02
+#define R_C4		0x04
+#define R_C5		0x06
+
+#define R_II_B1		0x06
+#define R_II_B2		0x02
+#define R_II_B3		0x00
+
+#define CMD_VER		0x96
+
+#define CH348_RX_PORT_CHUNK_LENGTH	32
+#define CH348_RX_PORT_MAX_LENGTH	30
+
+struct ch348_rxbuf {
+	u8 port;
+	u8 length;
+	u8 data[CH348_RX_PORT_MAX_LENGTH];
+} __packed;
+
+struct ch348_txbuf {
+	u8 port;
+	__le16 length;
+	u8 data[];
+} __packed;
+
+#define CH348_TX_HDRSIZE offsetof(struct ch348_txbuf, data)
+
+struct ch348_initbuf {
+	u8 cmd;
+	u8 reg;
+	u8 port;
+	__be32 baudrate;
+	u8 format;
+	u8 paritytype;
+	u8 databits;
+	u8 rate;
+	u8 unknown;
+} __packed;
+
+#define CH348_MAXPORT 8
+
+/*
+ * The CH348 multiplexes rx & tx into a pair of Bulk USB endpoints for
+ * the 8 serial ports, and another pair of Bulk USB endpoints to
+ * set port settings and receive port status events.
+ *
+ * The USB serial cores ties every Bulk endpoints pairs to each ports,
+ * but in our case it will set port 0 with the rx/tx endpoints
+ * and port 1 with the setup/status endpoints.
+ *
+ * To still take advantage of the generic code, we (re-)initialize
+ * the USB serial port structure with the correct USB endpoint
+ * for read and write, and write proper process_read_urb()
+ * and prepare_write_buffer() to correctly (de-)multiplex data.
+ * Also we use a custom write() implementation to wait until the buffer
+ * has been fully transmitted to prevent TX buffer overruns.
+ */
+
+/*
+ * struct ch348_port - per-port information
+ * @uartmode:		UART port current mode
+ * @write_completion:	completion event when the TX buffer has been written out
+ */
+struct ch348_port {
+	u8 uartmode;
+	struct completion write_completion;
+};
+
+/*
+ * struct ch348 - main container for all this driver information
+ * @udev:		pointer to the CH348 USB device
+ * @ports:		List of per-port information
+ * @serial:		pointer to the serial structure
+ * @write_lock:		protect against concurrent writes so we don't lose data
+ * @cmd_ep:		endpoint number for configure operations
+ * @status_urb:		URB for status
+ * @status_buffer:	buffer used by status_urb
+ */
+struct ch348 {
+	struct usb_device *udev;
+	struct ch348_port ports[CH348_MAXPORT];
+	struct usb_serial *serial;
+
+	struct mutex write_lock;
+
+	int cmd_ep;
+
+	struct urb *status_urb;
+	u8 status_buffer[];
+};
+
+struct ch348_magic {
+	u8 action;
+	u8 reg;
+	u8 control;
+} __packed;
+
+struct ch348_status_entry {
+	u8 portnum:4;
+	u8 unused:4;
+	u8 reg_iir;
+	union {
+		u8 lsr_signal;
+		u8 modem_signal;
+		u8 init_data[10];
+	};
+} __packed;
+
+static void ch348_process_status_urb(struct urb *urb)
+{
+	struct ch348_status_entry *status_entry;
+	struct ch348 *ch348 = urb->context;
+	int ret, status = urb->status;
+	struct usb_serial_port *port;
+	unsigned int i, status_len;
+
+	switch (status) {
+	case 0:
+		/* success */
+		break;
+	case -ECONNRESET:
+	case -ENOENT:
+	case -ESHUTDOWN:
+		/* this urb is terminated, clean up */
+		dev_dbg(&urb->dev->dev, "%s - urb shutting down with status: %d\n",
+			__func__, status);
+		return;
+	default:
+		dev_err(&urb->dev->dev, "%s - nonzero urb status received: %d\n",
+			__func__, status);
+		goto exit;
+	}
+
+	if (urb->actual_length < 3) {
+		dev_warn(&ch348->udev->dev,
+			 "Received too short status buffer with %u bytes\n",
+			 urb->actual_length);
+		goto exit;
+	}
+
+	for (i = 0; i < urb->actual_length;) {
+		status_entry = urb->transfer_buffer + i;
+
+		if (status_entry->portnum >= CH348_MAXPORT) {
+			dev_warn(&ch348->udev->dev,
+				 "Invalid port %d in status entry\n",
+				 status_entry->portnum);
+			break;
+		}
+
+		port = ch348->serial->port[status_entry->portnum];
+		status_len = 3;
+
+		if (!status_entry->reg_iir) {
+			dev_dbg(&port->dev, "Ignoring status with zero reg_iir\n");
+		} else if (status_entry->reg_iir == R_INIT) {
+			status_len = 12;
+		} else if ((status_entry->reg_iir & 0x0f) == R_II_B1) {
+			if (status_entry->lsr_signal & CH348_LO)
+				port->icount.overrun++;
+			if (status_entry->lsr_signal & CH348_LP)
+				port->icount.parity++;
+			if (status_entry->lsr_signal & CH348_LF)
+				port->icount.frame++;
+			if (status_entry->lsr_signal & CH348_LF)
+				port->icount.brk++;
+		} else if ((status_entry->reg_iir & 0x0f) == R_II_B2) {
+			complete_all(&ch348->ports[status_entry->portnum].write_completion);
+		} else {
+			dev_warn(&port->dev,
+				 "Unsupported status with reg_iir 0x%02x\n",
+				 status_entry->reg_iir);
+		}
+
+		usb_serial_debug_data(&port->dev, __func__, status_len,
+				      urb->transfer_buffer + i);
+
+		i += status_len;
+	}
+
+exit:
+	ret = usb_submit_urb(urb, GFP_ATOMIC);
+	if (ret)
+		dev_err(&urb->dev->dev, "%s - usb_submit_urb failed; %d\n",
+			__func__, ret);
+}
+
+/*
+ * Some values came from vendor tree, and we have no meaning for them, this
+ * function simply use them.
+ */
+static int ch348_do_magic(struct ch348 *ch348, int portnum, u8 action, u8 reg, u8 control)
+{
+	struct ch348_magic *buffer;
+	int ret, len;
+
+	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	if (portnum < 4)
+		reg += 0x10 * portnum;
+	else
+		reg += 0x10 * (portnum - 4) + 0x08;
+
+	buffer->action = action;
+	buffer->reg = reg;
+	buffer->control = control;
+
+	ret = usb_bulk_msg(ch348->udev, ch348->cmd_ep, buffer, 3, &len,
+			   CH348_CMD_TIMEOUT);
+	if (ret)
+		dev_err(&ch348->udev->dev, "Failed to write magic err=%d\n", ret);
+
+	kfree(buffer);
+
+	return ret;
+}
+
+static int ch348_configure(struct ch348 *ch348, int portnum)
+{
+	int ret;
+
+	ret = ch348_do_magic(ch348, portnum, CMD_W_R, R_C2, 0x87);
+	if (ret)
+		return ret;
+
+	return ch348_do_magic(ch348, portnum, CMD_W_R, R_C4, 0x08);
+}
+
+static void ch348_process_read_urb(struct urb *urb)
+{
+	struct usb_serial_port *port = urb->context;
+	struct ch348 *ch348 = usb_get_serial_data(port->serial);
+	unsigned int portnum, usblen, i;
+	struct ch348_rxbuf *rxb;
+
+	if (urb->actual_length < 2) {
+		dev_dbg(&ch348->udev->dev, "Empty rx buffer\n");
+		return;
+	}
+
+	for (i = 0; i < urb->actual_length; i += CH348_RX_PORT_CHUNK_LENGTH) {
+		rxb = urb->transfer_buffer + i;
+		portnum = rxb->port;
+		if (portnum >= CH348_MAXPORT) {
+			dev_dbg(&ch348->udev->dev, "Invalid port %d\n", portnum);
+			break;
+		}
+
+		port = ch348->serial->port[portnum];
+
+		usblen = rxb->length;
+		if (usblen > CH348_RX_PORT_MAX_LENGTH) {
+			dev_dbg(&port->dev, "Invalid length %d for port %d\n",
+				usblen, portnum);
+			break;
+		}
+
+		tty_insert_flip_string(&port->port, rxb->data, usblen);
+		tty_flip_buffer_push(&port->port);
+		port->icount.rx += usblen;
+		usb_serial_debug_data(&port->dev, __func__, usblen, rxb->data);
+	}
+}
+
+static int ch348_prepare_write_buffer(struct usb_serial_port *port, void *dest, size_t size)
+{
+	struct ch348_txbuf *rxt = dest;
+	int count;
+
+	count = kfifo_out_locked(&port->write_fifo, rxt->data,
+				 size - CH348_TX_HDRSIZE, &port->lock);
+
+	rxt->port = port->port_number;
+	rxt->length = cpu_to_le16(count);
+
+	return count + CH348_TX_HDRSIZE;
+}
+
+static int ch348_write(struct tty_struct *tty, struct usb_serial_port *port,
+		       const unsigned char *buf, int count)
+{
+	struct ch348 *ch348 = usb_get_serial_data(port->serial);
+	struct ch348_port *ch348_port = &ch348->ports[port->port_number];
+	int ret, max_tx_size;
+
+	if (tty_get_baud_rate(tty) < 9600 && count >= 128)
+		/*
+		 * Writing larger buffers can take longer than the hardware
+		 * allows before discarding the write buffer. Limit the
+		 * transfer size in such cases.
+		 * These values have been found by empirical testing.
+		 */
+		max_tx_size = 128;
+	else
+		/*
+		* Only ingest as many bytes as we can transfer with one URB at
+		* a time. Once an URB has been written we need to wait for the
+		* R_II_B2 status event before we are allowed to send more data.
+		* If we ingest more data then usb_serial_generic_write() will
+		* internally try to process as much data as possible with any
+		* number of URBs without giving us the chance to wait in
+		* between transfers.
+		*/
+		max_tx_size = port->bulk_out_size - CH348_TX_HDRSIZE;
+
+	reinit_completion(&ch348_port->write_completion);
+
+	mutex_lock(&ch348->write_lock);
+
+	/*
+	 * For any (remaining) bytes that we did not transfer TTY core will
+	 * call us again, with the buffer and count adjusted to the remaining
+	 * data.
+	 */
+	ret = usb_serial_generic_write(tty, port, buf, min(count, max_tx_size));
+
+	mutex_unlock(&ch348->write_lock);
+
+	if (ret <= 0)
+		return ret;
+
+	if (!wait_for_completion_interruptible_timeout(&ch348_port->write_completion,
+						       msecs_to_jiffies(CH348_CMD_TIMEOUT))) {
+		dev_err_console(port, "Failed to wait for TX buffer flush\n");
+		return -ETIMEDOUT;
+	}
+
+	return ret;
+}
+
+static int ch348_set_uartmode(struct ch348 *ch348, int portnum, u8 mode)
+{
+	int ret;
+
+	if (ch348->ports[portnum].uartmode == M_NOR && mode == M_HF) {
+		ret = ch348_do_magic(ch348, portnum, CMD_W_BR, R_C4, 0x51);
+		if (ret)
+			return ret;
+		ch348->ports[portnum].uartmode = M_HF;
+	}
+
+	if (ch348->ports[portnum].uartmode == M_HF && mode == M_NOR) {
+		ret = ch348_do_magic(ch348, portnum, CMD_W_BR, R_C4, 0x50);
+		if (ret)
+			return ret;
+		ch348->ports[portnum].uartmode = M_NOR;
+	}
+	return 0;
+}
+
+static void ch348_set_termios(struct tty_struct *tty, struct usb_serial_port *port,
+			      const struct ktermios *termios_old)
+{
+	struct ch348 *ch348 = usb_get_serial_data(port->serial);
+	struct ktermios *termios = &tty->termios;
+	int ret, portnum = port->port_number;
+	struct ch348_initbuf *buffer;
+	speed_t	baudrate;
+	u8 format;
+
+	if (termios_old && !tty_termios_hw_change(&tty->termios, termios_old))
+		return;
+
+	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
+	if (!buffer) {
+		if (termios_old)
+			tty->termios = *termios_old;
+		return;
+	}
+
+	/*
+	 * The datasheet states that only baud rates in range of 1200..6000000
+	 * are supported. Tests however show that even baud rates as low as 50
+	 * and as high as 12000000 are working in practice.
+	 */
+	baudrate = clamp(tty_get_baud_rate(tty), 50, 12000000);
+
+	format = termios->c_cflag & CSTOPB ? 2 : 1;
+
+	buffer->paritytype = 0;
+	if (termios->c_cflag & PARENB) {
+		if (termios->c_cflag & PARODD)
+			buffer->paritytype += 1;
+		else
+			buffer->paritytype += 2;
+		if  (termios->c_cflag & CMSPAR)
+			buffer->paritytype += 2;
+	}
+
+	switch (C_CSIZE(tty)) {
+	case CS5:
+		buffer->databits = 5;
+		break;
+	case CS6:
+		buffer->databits = 6;
+		break;
+	case CS7:
+		buffer->databits = 7;
+		break;
+	case CS8:
+	default:
+		buffer->databits = 8;
+		break;
+	}
+	buffer->cmd = CMD_WB_E | (portnum & 0x0F);
+	buffer->reg = R_INIT;
+	buffer->port = portnum;
+	buffer->baudrate = cpu_to_be32(baudrate);
+
+	if (format == 2)
+		buffer->format = 0x02;
+	else if (format == 1)
+		buffer->format = 0x00;
+
+	buffer->rate = max_t(speed_t, 5, (10000 * 15 / baudrate) + 1);
+
+	ret = usb_bulk_msg(ch348->udev, ch348->cmd_ep, buffer,
+			   sizeof(*buffer), NULL, CH348_CMD_TIMEOUT);
+	if (ret < 0) {
+		dev_err(&ch348->udev->dev, "Failed to change line settings: err=%d\n",
+			ret);
+		goto out;
+	}
+
+	ret = ch348_do_magic(ch348, portnum, CMD_W_R, R_C1, 0x0F);
+	if (ret < 0)
+		goto out;
+
+	if (C_CRTSCTS(tty))
+		ret = ch348_set_uartmode(ch348, portnum, M_HF);
+	else
+		ret = ch348_set_uartmode(ch348, portnum, M_NOR);
+
+out:
+	kfree(buffer);
+}
+
+static int ch348_open(struct tty_struct *tty, struct usb_serial_port *port)
+{
+	struct ch348 *ch348 = usb_get_serial_data(port->serial);
+	int ret;
+
+	if (tty)
+		ch348_set_termios(tty, port, NULL);
+
+	ret = ch348_configure(ch348, port->port_number);
+	if (ret) {
+		dev_err(&ch348->udev->dev, "Fail to configure err=%d\n", ret);
+		return ret;
+	}
+
+	return usb_serial_generic_open(tty, port);
+}
+
+static int ch348_attach(struct usb_serial *serial)
+{
+	struct usb_endpoint_descriptor *epcmd, *epstatus;
+	struct usb_serial_port *port0 = serial->port[1];
+	struct usb_device *usb_dev = serial->dev;
+	int status_buffer_size, i, ret;
+	struct usb_interface *intf;
+	struct ch348 *ch348;
+
+	intf = usb_ifnum_to_if(usb_dev, 0);
+	epstatus = &intf->cur_altsetting->endpoint[2].desc;
+	epcmd = &intf->cur_altsetting->endpoint[3].desc;
+
+	status_buffer_size = usb_endpoint_maxp(epstatus);
+
+	ch348 = kzalloc(struct_size(ch348, status_buffer, status_buffer_size),
+			GFP_KERNEL);
+	if (!ch348)
+		return -ENOMEM;
+
+	usb_set_serial_data(serial, ch348);
+
+	ch348->udev = serial->dev;
+	ch348->serial = serial;
+	mutex_init(&ch348->write_lock);
+
+	for (i = 0; i < CH348_MAXPORT; i++)
+		init_completion(&ch348->ports[i].write_completion);
+
+	ch348->status_urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!ch348->status_urb) {
+		ret = -ENOMEM;
+		goto err_free_ch348;
+	}
+
+	usb_fill_bulk_urb(ch348->status_urb, ch348->udev,
+			  usb_rcvbulkpipe(ch348->udev, epstatus->bEndpointAddress),
+			  ch348->status_buffer, status_buffer_size,
+			  ch348_process_status_urb, ch348);
+
+	ret = usb_submit_urb(ch348->status_urb, GFP_KERNEL);
+	if (ret) {
+		dev_err(&ch348->udev->dev,
+			"%s - failed to submit status/interrupt urb %i\n",
+			__func__, ret);
+		goto err_free_status_urb;
+	}
+
+	ret = usb_serial_generic_submit_read_urbs(port0, GFP_KERNEL);
+	if (ret)
+		goto err_kill_status_urb;
+
+	ch348->cmd_ep = usb_sndbulkpipe(usb_dev, epcmd->bEndpointAddress);
+
+	return 0;
+
+err_kill_status_urb:
+	usb_kill_urb(ch348->status_urb);
+err_free_status_urb:
+	usb_free_urb(ch348->status_urb);
+err_free_ch348:
+	kfree(ch348);
+	return ret;
+}
+
+static void ch348_release(struct usb_serial *serial)
+{
+	struct ch348 *ch348 = usb_get_serial_data(serial);
+
+	usb_kill_urb(ch348->status_urb);
+	usb_free_urb(ch348->status_urb);
+
+	kfree(ch348);
+}
+
+static void ch348_print_version(struct usb_serial *serial)
+{
+	u8 *version_buf;
+	int ret;
+
+	version_buf = kzalloc(4, GFP_KERNEL);
+	if (!version_buf)
+		return;
+
+	ret = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
+			      CMD_VER,
+			      USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+			      0, 0, version_buf, 4, CH348_CMD_TIMEOUT);
+	if (ret < 0)
+		dev_dbg(&serial->dev->dev, "Failed to read CMD_VER: %d\n", ret);
+	else
+		dev_info(&serial->dev->dev, "Found WCH CH348%s\n",
+			 (version_buf[1] & 0x80) ? "Q" : "L");
+
+	kfree(version_buf);
+}
+
+static int ch348_probe(struct usb_serial *serial, const struct usb_device_id *id)
+{
+	struct usb_endpoint_descriptor *epread, *epwrite, *epstatus, *epcmd;
+	struct usb_device *usb_dev = serial->dev;
+	struct usb_interface *intf;
+	int ret;
+
+	intf = usb_ifnum_to_if(usb_dev, 0);
+
+	ret = usb_find_common_endpoints(intf->cur_altsetting, &epread, &epwrite,
+					NULL, NULL);
+	if (ret) {
+		dev_err(&serial->dev->dev, "Failed to find basic endpoints ret=%d\n", ret);
+		return ret;
+	}
+
+	epstatus = &intf->cur_altsetting->endpoint[2].desc;
+	if (!usb_endpoint_is_bulk_in(epstatus)) {
+		dev_err(&serial->dev->dev, "Missing second bulk in (STATUS/INT)\n");
+		return -ENODEV;
+	}
+
+	epcmd = &intf->cur_altsetting->endpoint[3].desc;
+	if (!usb_endpoint_is_bulk_out(epcmd)) {
+		dev_err(&serial->dev->dev, "Missing second bulk out (CMD)\n");
+		return -ENODEV;
+	}
+
+	ch348_print_version(serial);
+
+	return 0;
+}
+
+static int ch348_calc_num_ports(struct usb_serial *serial,
+				struct usb_serial_endpoints *epds)
+{
+	int i;
+
+	for (i = 1; i < CH348_MAXPORT; ++i) {
+		epds->bulk_out[i] = epds->bulk_out[0];
+		epds->bulk_in[i] = epds->bulk_in[0];
+	}
+
+	epds->num_bulk_out = CH348_MAXPORT;
+	epds->num_bulk_in = CH348_MAXPORT;
+
+	return CH348_MAXPORT;
+}
+
+static int ch348_suspend(struct usb_serial *serial, pm_message_t message)
+{
+	struct ch348 *ch348 = usb_get_serial_data(serial);
+
+	usb_kill_urb(ch348->status_urb);
+
+	return 0;
+}
+
+static int ch348_resume(struct usb_serial *serial)
+{
+	struct ch348 *ch348 = usb_get_serial_data(serial);
+	int ret;
+
+	ret = usb_submit_urb(ch348->status_urb, GFP_KERNEL);
+	if (ret) {
+		dev_err(&ch348->udev->dev,
+			"%s - failed to submit status/interrupt urb %i\n",
+			__func__, ret);
+		return ret;
+	}
+
+	ret = usb_serial_generic_resume(serial);
+	if (ret)
+		usb_kill_urb(ch348->status_urb);
+
+	return ret;
+}
+
+static const struct usb_device_id ch348_ids[] = {
+	{ USB_DEVICE(0x1a86, 0x55d9), },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(usb, ch348_ids);
+
+static struct usb_serial_driver ch348_device = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "ch348",
+	},
+	.id_table =		ch348_ids,
+	.num_ports =		CH348_MAXPORT,
+	.num_bulk_in =		1,
+	.num_bulk_out =		1,
+	.open =			ch348_open,
+	.set_termios =		ch348_set_termios,
+	.process_read_urb =	ch348_process_read_urb,
+	.prepare_write_buffer =	ch348_prepare_write_buffer,
+	.write =		ch348_write,
+	.probe =		ch348_probe,
+	.calc_num_ports =	ch348_calc_num_ports,
+	.attach =		ch348_attach,
+	.release =		ch348_release,
+	.suspend =		ch348_suspend,
+	.resume =		ch348_resume,
+};
+
+static struct usb_serial_driver * const serial_drivers[] = {
+	&ch348_device, NULL
+};
+
+module_usb_serial_driver(serial_drivers, ch348_ids);
+
+MODULE_AUTHOR("Corentin Labbe <clabbe@baylibre.com>");
+MODULE_DESCRIPTION("USB CH348 Octo port serial converter driver");
+MODULE_LICENSE("GPL");
-- 
2.43.2

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[Martin Blumenstingl]

On Tue, May 7, 2024 at 3:15 PM Corentin Labbe <clabbe@baylibre.com> wrote:
>
> The CH348 is an USB octo port serial adapter.
> The device multiplexes all 8 ports in the same pair of Bulk endpoints.
> Since there is no public datasheet, unfortunately it remains some magic values
>
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
(on an Odroid-C1 which uses a dwc2 controller in host mode)

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[Paul Spooren]

[-- Attachment #1: Type: text/plain, Size: 22783 bytes --]



> On 7. May 2024, at 15:15, Corentin Labbe <clabbe@baylibre.com> wrote:
> 
> The CH348 is an USB octo port serial adapter.
> The device multiplexes all 8 ports in the same pair of Bulk endpoints.
> Since there is no public datasheet, unfortunately it remains some magic values
> 
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
Tested-by: Paul Spooren <mail@aparcar.org <mailto:mail@aparcar.org>>
(On BananaPi R3 running OpenWrt with YR-6008 module)

Patch is already being backported here https://github.com/openwrt/openwrt/pull/15733

> ---
> drivers/usb/serial/Kconfig  |   9 +
> drivers/usb/serial/Makefile |   1 +
> drivers/usb/serial/ch348.c  | 725 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 735 insertions(+)
> create mode 100644 drivers/usb/serial/ch348.c
> 
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> index ef8d1c73c754..1e1842656b54 100644
> --- a/drivers/usb/serial/Kconfig
> +++ b/drivers/usb/serial/Kconfig
> @@ -112,6 +112,15 @@ config USB_SERIAL_CH341
>  To compile this driver as a module, choose M here: the
>  module will be called ch341.
> 
> +config USB_SERIAL_CH348
> + tristate "USB Winchiphead CH348 Octo Port Serial Driver"
> + help
> +  Say Y here if you want to use a Winchiphead CH348 octo port
> +  USB to serial adapter.
> +
> +  To compile this driver as a module, choose M here: the
> +  module will be called ch348.
> +
> config USB_SERIAL_WHITEHEAT
> tristate "USB ConnectTech WhiteHEAT Serial Driver"
> select USB_EZUSB_FX2
> diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
> index c7bb1a88173e..d16ff59fde68 100644
> --- a/drivers/usb/serial/Makefile
> +++ b/drivers/usb/serial/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_USB_SERIAL_AIRCABLE) += aircable.o
> obj-$(CONFIG_USB_SERIAL_ARK3116) += ark3116.o
> obj-$(CONFIG_USB_SERIAL_BELKIN) += belkin_sa.o
> obj-$(CONFIG_USB_SERIAL_CH341) += ch341.o
> +obj-$(CONFIG_USB_SERIAL_CH348) += ch348.o
> obj-$(CONFIG_USB_SERIAL_CP210X) += cp210x.o
> obj-$(CONFIG_USB_SERIAL_CYBERJACK) += cyberjack.o
> obj-$(CONFIG_USB_SERIAL_CYPRESS_M8) += cypress_m8.o
> diff --git a/drivers/usb/serial/ch348.c b/drivers/usb/serial/ch348.c
> new file mode 100644
> index 000000000000..e437ba36bb5e
> --- /dev/null
> +++ b/drivers/usb/serial/ch348.c
> @@ -0,0 +1,725 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * USB serial driver for USB to Octal UARTs chip ch348.
> + *
> + * Copyright (C) 2022 Corentin Labbe <clabbe@baylibre.com>
> + * With the help of Neil Armstrong <neil.armstrong@linaro.org>
> + * and the help of Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/overflow.h>
> +#include <linux/serial.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_driver.h>
> +#include <linux/tty_flip.h>
> +#include <linux/usb.h>
> +#include <linux/usb/serial.h>
> +
> +#define CH348_CMD_TIMEOUT   2000
> +
> +#define CH348_CTO_D 0x01
> +#define CH348_CTO_R 0x02
> +
> +#define CH348_CTI_C 0x10
> +#define CH348_CTI_DSR 0x20
> +#define CH348_CTI_R 0x40
> +#define CH348_CTI_DCD 0x80
> +
> +#define CH348_LO 0x02
> +#define CH348_LP 0x04
> +#define CH348_LF 0x08
> +#define CH348_LB 0x10
> +
> +#define CMD_W_R 0xC0
> +#define CMD_W_BR 0x80
> +
> +#define CMD_WB_E 0x90
> +#define CMD_RB_E 0xC0
> +
> +#define M_NOR 0x00
> +#define M_HF 0x03
> +
> +#define R_MOD 0x97
> +#define R_IO_D 0x98
> +#define R_IO_O 0x99
> +#define R_IO_I 0x9b
> +#define R_TM_O 0x9c
> +#define R_INIT 0xa1
> +
> +#define R_C1 0x01
> +#define R_C2 0x02
> +#define R_C4 0x04
> +#define R_C5 0x06
> +
> +#define R_II_B1 0x06
> +#define R_II_B2 0x02
> +#define R_II_B3 0x00
> +
> +#define CMD_VER 0x96
> +
> +#define CH348_RX_PORT_CHUNK_LENGTH 32
> +#define CH348_RX_PORT_MAX_LENGTH 30
> +
> +struct ch348_rxbuf {
> + u8 port;
> + u8 length;
> + u8 data[CH348_RX_PORT_MAX_LENGTH];
> +} __packed;
> +
> +struct ch348_txbuf {
> + u8 port;
> + __le16 length;
> + u8 data[];
> +} __packed;
> +
> +#define CH348_TX_HDRSIZE offsetof(struct ch348_txbuf, data)
> +
> +struct ch348_initbuf {
> + u8 cmd;
> + u8 reg;
> + u8 port;
> + __be32 baudrate;
> + u8 format;
> + u8 paritytype;
> + u8 databits;
> + u8 rate;
> + u8 unknown;
> +} __packed;
> +
> +#define CH348_MAXPORT 8
> +
> +/*
> + * The CH348 multiplexes rx & tx into a pair of Bulk USB endpoints for
> + * the 8 serial ports, and another pair of Bulk USB endpoints to
> + * set port settings and receive port status events.
> + *
> + * The USB serial cores ties every Bulk endpoints pairs to each ports,
> + * but in our case it will set port 0 with the rx/tx endpoints
> + * and port 1 with the setup/status endpoints.
> + *
> + * To still take advantage of the generic code, we (re-)initialize
> + * the USB serial port structure with the correct USB endpoint
> + * for read and write, and write proper process_read_urb()
> + * and prepare_write_buffer() to correctly (de-)multiplex data.
> + * Also we use a custom write() implementation to wait until the buffer
> + * has been fully transmitted to prevent TX buffer overruns.
> + */
> +
> +/*
> + * struct ch348_port - per-port information
> + * @uartmode: UART port current mode
> + * @write_completion: completion event when the TX buffer has been written out
> + */
> +struct ch348_port {
> + u8 uartmode;
> + struct completion write_completion;
> +};
> +
> +/*
> + * struct ch348 - main container for all this driver information
> + * @udev: pointer to the CH348 USB device
> + * @ports: List of per-port information
> + * @serial: pointer to the serial structure
> + * @write_lock: protect against concurrent writes so we don't lose data
> + * @cmd_ep: endpoint number for configure operations
> + * @status_urb: URB for status
> + * @status_buffer: buffer used by status_urb
> + */
> +struct ch348 {
> + struct usb_device *udev;
> + struct ch348_port ports[CH348_MAXPORT];
> + struct usb_serial *serial;
> +
> + struct mutex write_lock;
> +
> + int cmd_ep;
> +
> + struct urb *status_urb;
> + u8 status_buffer[];
> +};
> +
> +struct ch348_magic {
> + u8 action;
> + u8 reg;
> + u8 control;
> +} __packed;
> +
> +struct ch348_status_entry {
> + u8 portnum:4;
> + u8 unused:4;
> + u8 reg_iir;
> + union {
> + u8 lsr_signal;
> + u8 modem_signal;
> + u8 init_data[10];
> + };
> +} __packed;
> +
> +static void ch348_process_status_urb(struct urb *urb)
> +{
> + struct ch348_status_entry *status_entry;
> + struct ch348 *ch348 = urb->context;
> + int ret, status = urb->status;
> + struct usb_serial_port *port;
> + unsigned int i, status_len;
> +
> + switch (status) {
> + case 0:
> + /* success */
> + break;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + /* this urb is terminated, clean up */
> + dev_dbg(&urb->dev->dev, "%s - urb shutting down with status: %d\n",
> + __func__, status);
> + return;
> + default:
> + dev_err(&urb->dev->dev, "%s - nonzero urb status received: %d\n",
> + __func__, status);
> + goto exit;
> + }
> +
> + if (urb->actual_length < 3) {
> + dev_warn(&ch348->udev->dev,
> + "Received too short status buffer with %u bytes\n",
> + urb->actual_length);
> + goto exit;
> + }
> +
> + for (i = 0; i < urb->actual_length;) {
> + status_entry = urb->transfer_buffer + i;
> +
> + if (status_entry->portnum >= CH348_MAXPORT) {
> + dev_warn(&ch348->udev->dev,
> + "Invalid port %d in status entry\n",
> + status_entry->portnum);
> + break;
> + }
> +
> + port = ch348->serial->port[status_entry->portnum];
> + status_len = 3;
> +
> + if (!status_entry->reg_iir) {
> + dev_dbg(&port->dev, "Ignoring status with zero reg_iir\n");
> + } else if (status_entry->reg_iir == R_INIT) {
> + status_len = 12;
> + } else if ((status_entry->reg_iir & 0x0f) == R_II_B1) {
> + if (status_entry->lsr_signal & CH348_LO)
> + port->icount.overrun++;
> + if (status_entry->lsr_signal & CH348_LP)
> + port->icount.parity++;
> + if (status_entry->lsr_signal & CH348_LF)
> + port->icount.frame++;
> + if (status_entry->lsr_signal & CH348_LF)
> + port->icount.brk++;
> + } else if ((status_entry->reg_iir & 0x0f) == R_II_B2) {
> + complete_all(&ch348->ports[status_entry->portnum].write_completion);
> + } else {
> + dev_warn(&port->dev,
> + "Unsupported status with reg_iir 0x%02x\n",
> + status_entry->reg_iir);
> + }
> +
> + usb_serial_debug_data(&port->dev, __func__, status_len,
> +      urb->transfer_buffer + i);
> +
> + i += status_len;
> + }
> +
> +exit:
> + ret = usb_submit_urb(urb, GFP_ATOMIC);
> + if (ret)
> + dev_err(&urb->dev->dev, "%s - usb_submit_urb failed; %d\n",
> + __func__, ret);
> +}
> +
> +/*
> + * Some values came from vendor tree, and we have no meaning for them, this
> + * function simply use them.
> + */
> +static int ch348_do_magic(struct ch348 *ch348, int portnum, u8 action, u8 reg, u8 control)
> +{
> + struct ch348_magic *buffer;
> + int ret, len;
> +
> + buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> + if (!buffer)
> + return -ENOMEM;
> +
> + if (portnum < 4)
> + reg += 0x10 * portnum;
> + else
> + reg += 0x10 * (portnum - 4) + 0x08;
> +
> + buffer->action = action;
> + buffer->reg = reg;
> + buffer->control = control;
> +
> + ret = usb_bulk_msg(ch348->udev, ch348->cmd_ep, buffer, 3, &len,
> +   CH348_CMD_TIMEOUT);
> + if (ret)
> + dev_err(&ch348->udev->dev, "Failed to write magic err=%d\n", ret);
> +
> + kfree(buffer);
> +
> + return ret;
> +}
> +
> +static int ch348_configure(struct ch348 *ch348, int portnum)
> +{
> + int ret;
> +
> + ret = ch348_do_magic(ch348, portnum, CMD_W_R, R_C2, 0x87);
> + if (ret)
> + return ret;
> +
> + return ch348_do_magic(ch348, portnum, CMD_W_R, R_C4, 0x08);
> +}
> +
> +static void ch348_process_read_urb(struct urb *urb)
> +{
> + struct usb_serial_port *port = urb->context;
> + struct ch348 *ch348 = usb_get_serial_data(port->serial);
> + unsigned int portnum, usblen, i;
> + struct ch348_rxbuf *rxb;
> +
> + if (urb->actual_length < 2) {
> + dev_dbg(&ch348->udev->dev, "Empty rx buffer\n");
> + return;
> + }
> +
> + for (i = 0; i < urb->actual_length; i += CH348_RX_PORT_CHUNK_LENGTH) {
> + rxb = urb->transfer_buffer + i;
> + portnum = rxb->port;
> + if (portnum >= CH348_MAXPORT) {
> + dev_dbg(&ch348->udev->dev, "Invalid port %d\n", portnum);
> + break;
> + }
> +
> + port = ch348->serial->port[portnum];
> +
> + usblen = rxb->length;
> + if (usblen > CH348_RX_PORT_MAX_LENGTH) {
> + dev_dbg(&port->dev, "Invalid length %d for port %d\n",
> + usblen, portnum);
> + break;
> + }
> +
> + tty_insert_flip_string(&port->port, rxb->data, usblen);
> + tty_flip_buffer_push(&port->port);
> + port->icount.rx += usblen;
> + usb_serial_debug_data(&port->dev, __func__, usblen, rxb->data);
> + }
> +}
> +
> +static int ch348_prepare_write_buffer(struct usb_serial_port *port, void *dest, size_t size)
> +{
> + struct ch348_txbuf *rxt = dest;
> + int count;
> +
> + count = kfifo_out_locked(&port->write_fifo, rxt->data,
> + size - CH348_TX_HDRSIZE, &port->lock);
> +
> + rxt->port = port->port_number;
> + rxt->length = cpu_to_le16(count);
> +
> + return count + CH348_TX_HDRSIZE;
> +}
> +
> +static int ch348_write(struct tty_struct *tty, struct usb_serial_port *port,
> +       const unsigned char *buf, int count)
> +{
> + struct ch348 *ch348 = usb_get_serial_data(port->serial);
> + struct ch348_port *ch348_port = &ch348->ports[port->port_number];
> + int ret, max_tx_size;
> +
> + if (tty_get_baud_rate(tty) < 9600 && count >= 128)
> + /*
> + * Writing larger buffers can take longer than the hardware
> + * allows before discarding the write buffer. Limit the
> + * transfer size in such cases.
> + * These values have been found by empirical testing.
> + */
> + max_tx_size = 128;
> + else
> + /*
> + * Only ingest as many bytes as we can transfer with one URB at
> + * a time. Once an URB has been written we need to wait for the
> + * R_II_B2 status event before we are allowed to send more data.
> + * If we ingest more data then usb_serial_generic_write() will
> + * internally try to process as much data as possible with any
> + * number of URBs without giving us the chance to wait in
> + * between transfers.
> + */
> + max_tx_size = port->bulk_out_size - CH348_TX_HDRSIZE;
> +
> + reinit_completion(&ch348_port->write_completion);
> +
> + mutex_lock(&ch348->write_lock);
> +
> + /*
> + * For any (remaining) bytes that we did not transfer TTY core will
> + * call us again, with the buffer and count adjusted to the remaining
> + * data.
> + */
> + ret = usb_serial_generic_write(tty, port, buf, min(count, max_tx_size));
> +
> + mutex_unlock(&ch348->write_lock);
> +
> + if (ret <= 0)
> + return ret;
> +
> + if (!wait_for_completion_interruptible_timeout(&ch348_port->write_completion,
> +       msecs_to_jiffies(CH348_CMD_TIMEOUT))) {
> + dev_err_console(port, "Failed to wait for TX buffer flush\n");
> + return -ETIMEDOUT;
> + }
> +
> + return ret;
> +}
> +
> +static int ch348_set_uartmode(struct ch348 *ch348, int portnum, u8 mode)
> +{
> + int ret;
> +
> + if (ch348->ports[portnum].uartmode == M_NOR && mode == M_HF) {
> + ret = ch348_do_magic(ch348, portnum, CMD_W_BR, R_C4, 0x51);
> + if (ret)
> + return ret;
> + ch348->ports[portnum].uartmode = M_HF;
> + }
> +
> + if (ch348->ports[portnum].uartmode == M_HF && mode == M_NOR) {
> + ret = ch348_do_magic(ch348, portnum, CMD_W_BR, R_C4, 0x50);
> + if (ret)
> + return ret;
> + ch348->ports[portnum].uartmode = M_NOR;
> + }
> + return 0;
> +}
> +
> +static void ch348_set_termios(struct tty_struct *tty, struct usb_serial_port *port,
> +      const struct ktermios *termios_old)
> +{
> + struct ch348 *ch348 = usb_get_serial_data(port->serial);
> + struct ktermios *termios = &tty->termios;
> + int ret, portnum = port->port_number;
> + struct ch348_initbuf *buffer;
> + speed_t baudrate;
> + u8 format;
> +
> + if (termios_old && !tty_termios_hw_change(&tty->termios, termios_old))
> + return;
> +
> + buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> + if (!buffer) {
> + if (termios_old)
> + tty->termios = *termios_old;
> + return;
> + }
> +
> + /*
> + * The datasheet states that only baud rates in range of 1200..6000000
> + * are supported. Tests however show that even baud rates as low as 50
> + * and as high as 12000000 are working in practice.
> + */
> + baudrate = clamp(tty_get_baud_rate(tty), 50, 12000000);
> +
> + format = termios->c_cflag & CSTOPB ? 2 : 1;
> +
> + buffer->paritytype = 0;
> + if (termios->c_cflag & PARENB) {
> + if (termios->c_cflag & PARODD)
> + buffer->paritytype += 1;
> + else
> + buffer->paritytype += 2;
> + if  (termios->c_cflag & CMSPAR)
> + buffer->paritytype += 2;
> + }
> +
> + switch (C_CSIZE(tty)) {
> + case CS5:
> + buffer->databits = 5;
> + break;
> + case CS6:
> + buffer->databits = 6;
> + break;
> + case CS7:
> + buffer->databits = 7;
> + break;
> + case CS8:
> + default:
> + buffer->databits = 8;
> + break;
> + }
> + buffer->cmd = CMD_WB_E | (portnum & 0x0F);
> + buffer->reg = R_INIT;
> + buffer->port = portnum;
> + buffer->baudrate = cpu_to_be32(baudrate);
> +
> + if (format == 2)
> + buffer->format = 0x02;
> + else if (format == 1)
> + buffer->format = 0x00;
> +
> + buffer->rate = max_t(speed_t, 5, (10000 * 15 / baudrate) + 1);
> +
> + ret = usb_bulk_msg(ch348->udev, ch348->cmd_ep, buffer,
> +   sizeof(*buffer), NULL, CH348_CMD_TIMEOUT);
> + if (ret < 0) {
> + dev_err(&ch348->udev->dev, "Failed to change line settings: err=%d\n",
> + ret);
> + goto out;
> + }
> +
> + ret = ch348_do_magic(ch348, portnum, CMD_W_R, R_C1, 0x0F);
> + if (ret < 0)
> + goto out;
> +
> + if (C_CRTSCTS(tty))
> + ret = ch348_set_uartmode(ch348, portnum, M_HF);
> + else
> + ret = ch348_set_uartmode(ch348, portnum, M_NOR);
> +
> +out:
> + kfree(buffer);
> +}
> +
> +static int ch348_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> + struct ch348 *ch348 = usb_get_serial_data(port->serial);
> + int ret;
> +
> + if (tty)
> + ch348_set_termios(tty, port, NULL);
> +
> + ret = ch348_configure(ch348, port->port_number);
> + if (ret) {
> + dev_err(&ch348->udev->dev, "Fail to configure err=%d\n", ret);
> + return ret;
> + }
> +
> + return usb_serial_generic_open(tty, port);
> +}
> +
> +static int ch348_attach(struct usb_serial *serial)
> +{
> + struct usb_endpoint_descriptor *epcmd, *epstatus;
> + struct usb_serial_port *port0 = serial->port[1];
> + struct usb_device *usb_dev = serial->dev;
> + int status_buffer_size, i, ret;
> + struct usb_interface *intf;
> + struct ch348 *ch348;
> +
> + intf = usb_ifnum_to_if(usb_dev, 0);
> + epstatus = &intf->cur_altsetting->endpoint[2].desc;
> + epcmd = &intf->cur_altsetting->endpoint[3].desc;
> +
> + status_buffer_size = usb_endpoint_maxp(epstatus);
> +
> + ch348 = kzalloc(struct_size(ch348, status_buffer, status_buffer_size),
> + GFP_KERNEL);
> + if (!ch348)
> + return -ENOMEM;
> +
> + usb_set_serial_data(serial, ch348);
> +
> + ch348->udev = serial->dev;
> + ch348->serial = serial;
> + mutex_init(&ch348->write_lock);
> +
> + for (i = 0; i < CH348_MAXPORT; i++)
> + init_completion(&ch348->ports[i].write_completion);
> +
> + ch348->status_urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!ch348->status_urb) {
> + ret = -ENOMEM;
> + goto err_free_ch348;
> + }
> +
> + usb_fill_bulk_urb(ch348->status_urb, ch348->udev,
> +  usb_rcvbulkpipe(ch348->udev, epstatus->bEndpointAddress),
> +  ch348->status_buffer, status_buffer_size,
> +  ch348_process_status_urb, ch348);
> +
> + ret = usb_submit_urb(ch348->status_urb, GFP_KERNEL);
> + if (ret) {
> + dev_err(&ch348->udev->dev,
> + "%s - failed to submit status/interrupt urb %i\n",
> + __func__, ret);
> + goto err_free_status_urb;
> + }
> +
> + ret = usb_serial_generic_submit_read_urbs(port0, GFP_KERNEL);
> + if (ret)
> + goto err_kill_status_urb;
> +
> + ch348->cmd_ep = usb_sndbulkpipe(usb_dev, epcmd->bEndpointAddress);
> +
> + return 0;
> +
> +err_kill_status_urb:
> + usb_kill_urb(ch348->status_urb);
> +err_free_status_urb:
> + usb_free_urb(ch348->status_urb);
> +err_free_ch348:
> + kfree(ch348);
> + return ret;
> +}
> +
> +static void ch348_release(struct usb_serial *serial)
> +{
> + struct ch348 *ch348 = usb_get_serial_data(serial);
> +
> + usb_kill_urb(ch348->status_urb);
> + usb_free_urb(ch348->status_urb);
> +
> + kfree(ch348);
> +}
> +
> +static void ch348_print_version(struct usb_serial *serial)
> +{
> + u8 *version_buf;
> + int ret;
> +
> + version_buf = kzalloc(4, GFP_KERNEL);
> + if (!version_buf)
> + return;
> +
> + ret = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> +      CMD_VER,
> +      USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> +      0, 0, version_buf, 4, CH348_CMD_TIMEOUT);
> + if (ret < 0)
> + dev_dbg(&serial->dev->dev, "Failed to read CMD_VER: %d\n", ret);
> + else
> + dev_info(&serial->dev->dev, "Found WCH CH348%s\n",
> + (version_buf[1] & 0x80) ? "Q" : "L");
> +
> + kfree(version_buf);
> +}
> +
> +static int ch348_probe(struct usb_serial *serial, const struct usb_device_id *id)
> +{
> + struct usb_endpoint_descriptor *epread, *epwrite, *epstatus, *epcmd;
> + struct usb_device *usb_dev = serial->dev;
> + struct usb_interface *intf;
> + int ret;
> +
> + intf = usb_ifnum_to_if(usb_dev, 0);
> +
> + ret = usb_find_common_endpoints(intf->cur_altsetting, &epread, &epwrite,
> + NULL, NULL);
> + if (ret) {
> + dev_err(&serial->dev->dev, "Failed to find basic endpoints ret=%d\n", ret);
> + return ret;
> + }
> +
> + epstatus = &intf->cur_altsetting->endpoint[2].desc;
> + if (!usb_endpoint_is_bulk_in(epstatus)) {
> + dev_err(&serial->dev->dev, "Missing second bulk in (STATUS/INT)\n");
> + return -ENODEV;
> + }
> +
> + epcmd = &intf->cur_altsetting->endpoint[3].desc;
> + if (!usb_endpoint_is_bulk_out(epcmd)) {
> + dev_err(&serial->dev->dev, "Missing second bulk out (CMD)\n");
> + return -ENODEV;
> + }
> +
> + ch348_print_version(serial);
> +
> + return 0;
> +}
> +
> +static int ch348_calc_num_ports(struct usb_serial *serial,
> + struct usb_serial_endpoints *epds)
> +{
> + int i;
> +
> + for (i = 1; i < CH348_MAXPORT; ++i) {
> + epds->bulk_out[i] = epds->bulk_out[0];
> + epds->bulk_in[i] = epds->bulk_in[0];
> + }
> +
> + epds->num_bulk_out = CH348_MAXPORT;
> + epds->num_bulk_in = CH348_MAXPORT;
> +
> + return CH348_MAXPORT;
> +}
> +
> +static int ch348_suspend(struct usb_serial *serial, pm_message_t message)
> +{
> + struct ch348 *ch348 = usb_get_serial_data(serial);
> +
> + usb_kill_urb(ch348->status_urb);
> +
> + return 0;
> +}
> +
> +static int ch348_resume(struct usb_serial *serial)
> +{
> + struct ch348 *ch348 = usb_get_serial_data(serial);
> + int ret;
> +
> + ret = usb_submit_urb(ch348->status_urb, GFP_KERNEL);
> + if (ret) {
> + dev_err(&ch348->udev->dev,
> + "%s - failed to submit status/interrupt urb %i\n",
> + __func__, ret);
> + return ret;
> + }
> +
> + ret = usb_serial_generic_resume(serial);
> + if (ret)
> + usb_kill_urb(ch348->status_urb);
> +
> + return ret;
> +}
> +
> +static const struct usb_device_id ch348_ids[] = {
> + { USB_DEVICE(0x1a86, 0x55d9), },
> + { /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(usb, ch348_ids);
> +
> +static struct usb_serial_driver ch348_device = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "ch348",
> + },
> + .id_table = ch348_ids,
> + .num_ports = CH348_MAXPORT,
> + .num_bulk_in = 1,
> + .num_bulk_out = 1,
> + .open = ch348_open,
> + .set_termios = ch348_set_termios,
> + .process_read_urb = ch348_process_read_urb,
> + .prepare_write_buffer = ch348_prepare_write_buffer,
> + .write = ch348_write,
> + .probe = ch348_probe,
> + .calc_num_ports = ch348_calc_num_ports,
> + .attach = ch348_attach,
> + .release = ch348_release,
> + .suspend = ch348_suspend,
> + .resume = ch348_resume,
> +};
> +
> +static struct usb_serial_driver * const serial_drivers[] = {
> + &ch348_device, NULL
> +};
> +
> +module_usb_serial_driver(serial_drivers, ch348_ids);
> +
> +MODULE_AUTHOR("Corentin Labbe <clabbe@baylibre.com>");
> +MODULE_DESCRIPTION("USB CH348 Octo port serial converter driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.43.2
> 
> 


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[Martin Blumenstingl]

Hi Paul, Hi Johan,

On Wed, Jun 19, 2024 at 3:35 PM Paul Spooren <mail@aparcar.org> wrote:
> > On 7. May 2024, at 15:15, Corentin Labbe <clabbe@baylibre.com> wrote:
> >
> > The CH348 is an USB octo port serial adapter.
> > The device multiplexes all 8 ports in the same pair of Bulk endpoints.
> > Since there is no public datasheet, unfortunately it remains some magic values
> >
> > Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> Tested-by: Paul Spooren <mail@aparcar.org <mailto:mail@aparcar.org>>
Thanks Paul!
Johan, is the Tested-by something that you can fix up while applying?

Speaking of applying this patch, it would be great to have it queued
for Linux 6.11.
If you have any questions or review comments then please let Corentin
and me know.


Best regards,
Martin

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[Johan Hovold]

On Sat, Jun 22, 2024 at 09:00:17PM +0200, Martin Blumenstingl wrote:
> On Wed, Jun 19, 2024 at 3:35 PM Paul Spooren <mail@aparcar.org> wrote:
> > > On 7. May 2024, at 15:15, Corentin Labbe <clabbe@baylibre.com> wrote:
> > >
> > > The CH348 is an USB octo port serial adapter.
> > > The device multiplexes all 8 ports in the same pair of Bulk endpoints.
> > > Since there is no public datasheet, unfortunately it remains some magic values
> > >
> > > Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> > Tested-by: Paul Spooren <mail@aparcar.org <mailto:mail@aparcar.org>>

> Thanks Paul!
> Johan, is the Tested-by something that you can fix up while applying?

Sure.

> Speaking of applying this patch, it would be great to have it queued
> for Linux 6.11.
> If you have any questions or review comments then please let Corentin
> and me know.

If you think the outstanding issues have been addressed now then I'll
find some time to review it again.

Johan

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[Martin Blumenstingl]

On Thu, Jun 27, 2024 at 9:14 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Sat, Jun 22, 2024 at 09:00:17PM +0200, Martin Blumenstingl wrote:
> > On Wed, Jun 19, 2024 at 3:35 PM Paul Spooren <mail@aparcar.org> wrote:
> > > > On 7. May 2024, at 15:15, Corentin Labbe <clabbe@baylibre.com> wrote:
> > > >
> > > > The CH348 is an USB octo port serial adapter.
> > > > The device multiplexes all 8 ports in the same pair of Bulk endpoints.
> > > > Since there is no public datasheet, unfortunately it remains some magic values
> > > >
> > > > Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> > > Tested-by: Paul Spooren <mail@aparcar.org <mailto:mail@aparcar.org>>
>
> > Thanks Paul!
> > Johan, is the Tested-by something that you can fix up while applying?
>
> Sure.
Great, thank you!

> > Speaking of applying this patch, it would be great to have it queued
> > for Linux 6.11.
> > If you have any questions or review comments then please let Corentin
> > and me know.
>
> If you think the outstanding issues have been addressed now then I'll
> find some time to review it again.
The issue reported originally by Nicolas and then confirmed by
Corentin in [0] is indeed fixed with this v7.
I am not aware of any other functional issues.

There's additional features that the hardware supports which the
driver doesn't yet - for example modem control.
Those features can be implemented later - as necessary.


Best regards,
Martin


[0] https://lore.kernel.org/lkml/ZVtRNZmCMImCT9sN@Red/

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[Johan Hovold]

On Tue, May 07, 2024 at 01:15:22PM +0000, Corentin Labbe wrote:
> The CH348 is an USB octo port serial adapter.
> The device multiplexes all 8 ports in the same pair of Bulk endpoints.
> Since there is no public datasheet, unfortunately it remains some magic values

Could you please include a pointer to the vendor driver (which I assumed
you based this on)?

> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> ---
>  drivers/usb/serial/Kconfig  |   9 +
>  drivers/usb/serial/Makefile |   1 +
>  drivers/usb/serial/ch348.c  | 725 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 735 insertions(+)
>  create mode 100644 drivers/usb/serial/ch348.c
> 
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> index ef8d1c73c754..1e1842656b54 100644
> --- a/drivers/usb/serial/Kconfig
> +++ b/drivers/usb/serial/Kconfig
> @@ -112,6 +112,15 @@ config USB_SERIAL_CH341
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ch341.
>  
> +config USB_SERIAL_CH348
> +	tristate "USB Winchiphead CH348 Octo Port Serial Driver"
> +	help
> +	  Say Y here if you want to use a Winchiphead CH348 octo port
> +	  USB to serial adapter.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ch348.
> +
>  config USB_SERIAL_WHITEHEAT
>  	tristate "USB ConnectTech WhiteHEAT Serial Driver"
>  	select USB_EZUSB_FX2
> diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
> index c7bb1a88173e..d16ff59fde68 100644
> --- a/drivers/usb/serial/Makefile
> +++ b/drivers/usb/serial/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_USB_SERIAL_AIRCABLE)		+= aircable.o
>  obj-$(CONFIG_USB_SERIAL_ARK3116)		+= ark3116.o
>  obj-$(CONFIG_USB_SERIAL_BELKIN)			+= belkin_sa.o
>  obj-$(CONFIG_USB_SERIAL_CH341)			+= ch341.o
> +obj-$(CONFIG_USB_SERIAL_CH348)			+= ch348.o
>  obj-$(CONFIG_USB_SERIAL_CP210X)			+= cp210x.o
>  obj-$(CONFIG_USB_SERIAL_CYBERJACK)		+= cyberjack.o
>  obj-$(CONFIG_USB_SERIAL_CYPRESS_M8)		+= cypress_m8.o
> diff --git a/drivers/usb/serial/ch348.c b/drivers/usb/serial/ch348.c
> new file mode 100644
> index 000000000000..e437ba36bb5e
> --- /dev/null
> +++ b/drivers/usb/serial/ch348.c
> @@ -0,0 +1,725 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * USB serial driver for USB to Octal UARTs chip ch348.
> + *
> + * Copyright (C) 2022 Corentin Labbe <clabbe@baylibre.com>

Do you need to include any copyrights from the vendor driver? It looks
like at least some of the code below was copied from somewhere (but
perhaps not that much).

> + * With the help of Neil Armstrong <neil.armstrong@linaro.org>
> + * and the help of Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/overflow.h>
> +#include <linux/serial.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_driver.h>
> +#include <linux/tty_flip.h>
> +#include <linux/usb.h>
> +#include <linux/usb/serial.h>
> +
> +#define CH348_CMD_TIMEOUT   2000
> +
> +#define CH348_CTO_D	0x01
> +#define CH348_CTO_R	0x02
> +
> +#define CH348_CTI_C	0x10
> +#define CH348_CTI_DSR	0x20
> +#define CH348_CTI_R	0x40
> +#define CH348_CTI_DCD	0x80
> +
> +#define CH348_LO	0x02
> +#define CH348_LP	0x04
> +#define CH348_LF	0x08
> +#define CH348_LB	0x10
> +
> +#define CMD_W_R		0xC0
> +#define CMD_W_BR	0x80
> +
> +#define CMD_WB_E	0x90
> +#define CMD_RB_E	0xC0
> +
> +#define M_NOR		0x00
> +#define M_HF		0x03
> +
> +#define R_MOD		0x97
> +#define R_IO_D		0x98
> +#define R_IO_O		0x99
> +#define R_IO_I		0x9b
> +#define R_TM_O		0x9c
> +#define R_INIT		0xa1
> +
> +#define R_C1		0x01
> +#define R_C2		0x02
> +#define R_C4		0x04
> +#define R_C5		0x06
> +
> +#define R_II_B1		0x06
> +#define R_II_B2		0x02
> +#define R_II_B3		0x00

This look like standard IIR masks (e.g. R_II_B2 would be Transmit
holding register empty).

> +
> +#define CMD_VER		0x96
> +
> +#define CH348_RX_PORT_CHUNK_LENGTH	32
> +#define CH348_RX_PORT_MAX_LENGTH	30
> +
> +struct ch348_rxbuf {
> +	u8 port;
> +	u8 length;
> +	u8 data[CH348_RX_PORT_MAX_LENGTH];
> +} __packed;
> +
> +struct ch348_txbuf {
> +	u8 port;
> +	__le16 length;
> +	u8 data[];
> +} __packed;
> +
> +#define CH348_TX_HDRSIZE offsetof(struct ch348_txbuf, data)
> +
> +struct ch348_initbuf {
> +	u8 cmd;
> +	u8 reg;
> +	u8 port;
> +	__be32 baudrate;
> +	u8 format;
> +	u8 paritytype;
> +	u8 databits;
> +	u8 rate;
> +	u8 unknown;
> +} __packed;
> +
> +#define CH348_MAXPORT 8
> +
> +/*
> + * The CH348 multiplexes rx & tx into a pair of Bulk USB endpoints for
> + * the 8 serial ports, and another pair of Bulk USB endpoints to
> + * set port settings and receive port status events.
> + *
> + * The USB serial cores ties every Bulk endpoints pairs to each ports,
> + * but in our case it will set port 0 with the rx/tx endpoints
> + * and port 1 with the setup/status endpoints.
> + *
> + * To still take advantage of the generic code, we (re-)initialize
> + * the USB serial port structure with the correct USB endpoint
> + * for read and write, and write proper process_read_urb()
> + * and prepare_write_buffer() to correctly (de-)multiplex data.
> + * Also we use a custom write() implementation to wait until the buffer
> + * has been fully transmitted to prevent TX buffer overruns.
> + */
> +
> +/*
> + * struct ch348_port - per-port information
> + * @uartmode:		UART port current mode
> + * @write_completion:	completion event when the TX buffer has been written out
> + */
> +struct ch348_port {
> +	u8 uartmode;
> +	struct completion write_completion;
> +};
> +
> +/*
> + * struct ch348 - main container for all this driver information
> + * @udev:		pointer to the CH348 USB device
> + * @ports:		List of per-port information

nit: lowercase L for consistency, but it's not really a list

> + * @serial:		pointer to the serial structure
> + * @write_lock:		protect against concurrent writes so we don't lose data
> + * @cmd_ep:		endpoint number for configure operations
> + * @status_urb:		URB for status
> + * @status_buffer:	buffer used by status_urb
> + */
> +struct ch348 {
> +	struct usb_device *udev;
> +	struct ch348_port ports[CH348_MAXPORT];
> +	struct usb_serial *serial;
> +
> +	struct mutex write_lock;
> +
> +	int cmd_ep;
> +
> +	struct urb *status_urb;
> +	u8 status_buffer[];

This buffer should be allocated separately as it is used for DMA.

> +};
> +
> +struct ch348_magic {
> +	u8 action;
> +	u8 reg;
> +	u8 control;
> +} __packed;
> +
> +struct ch348_status_entry {
> +	u8 portnum:4;
> +	u8 unused:4;

Don't use bitfields for things that go over the wire.

> +	u8 reg_iir;
> +	union {
> +		u8 lsr_signal;
> +		u8 modem_signal;
> +		u8 init_data[10];

This init_data field is never used and looks a bit odd. What is it used
for?

You mentioned that that the modem lines are not yet used either.

> +	};
> +} __packed;
> +
> +static void ch348_process_status_urb(struct urb *urb)
> +{
> +	struct ch348_status_entry *status_entry;
> +	struct ch348 *ch348 = urb->context;
> +	int ret, status = urb->status;

Avoid mixing and multitple declarations and initialisation like this.
One declaration per line is usually preferred unless it's related stuff.

> +	struct usb_serial_port *port;
> +	unsigned int i, status_len;
> +
> +	switch (status) {
> +	case 0:
> +		/* success */
> +		break;
> +	case -ECONNRESET:
> +	case -ENOENT:
> +	case -ESHUTDOWN:
> +		/* this urb is terminated, clean up */
> +		dev_dbg(&urb->dev->dev, "%s - urb shutting down with status: %d\n",
> +			__func__, status);
> +		return;
> +	default:
> +		dev_err(&urb->dev->dev, "%s - nonzero urb status received: %d\n",
> +			__func__, status);

Please avoid __func__ in error messages and spell out what happens
instead (e.g. "nonzero status-urb status received: %d").

> +		goto exit;
> +	}
> +
> +	if (urb->actual_length < 3) {
> +		dev_warn(&ch348->udev->dev,
> +			 "Received too short status buffer with %u bytes\n",
> +			 urb->actual_length);

This needs to be ratelimited at a minimum.

> +		goto exit;
> +	}
> +
> +	for (i = 0; i < urb->actual_length;) {
> +		status_entry = urb->transfer_buffer + i;
> +
> +		if (status_entry->portnum >= CH348_MAXPORT) {
> +			dev_warn(&ch348->udev->dev,
> +				 "Invalid port %d in status entry\n",
> +				 status_entry->portnum);

Same here.

> +			break;
> +		}
> +
> +		port = ch348->serial->port[status_entry->portnum];
> +		status_len = 3;
> +
> +		if (!status_entry->reg_iir) {
> +			dev_dbg(&port->dev, "Ignoring status with zero reg_iir\n");
> +		} else if (status_entry->reg_iir == R_INIT) {
> +			status_len = 12;

Ok, so this is the init data. What it is used for?

> +		} else if ((status_entry->reg_iir & 0x0f) == R_II_B1) {

Use a define for the mask.

> +			if (status_entry->lsr_signal & CH348_LO)
> +				port->icount.overrun++;
> +			if (status_entry->lsr_signal & CH348_LP)
> +				port->icount.parity++;
> +			if (status_entry->lsr_signal & CH348_LF)
> +				port->icount.frame++;
> +			if (status_entry->lsr_signal & CH348_LF)

Should you really count every framing error as a break? Looks like this
should have been CH348_LB.

> +				port->icount.brk++;
> +		} else if ((status_entry->reg_iir & 0x0f) == R_II_B2) {
> +			complete_all(&ch348->ports[status_entry->portnum].write_completion);
> +		} else {
> +			dev_warn(&port->dev,
> +				 "Unsupported status with reg_iir 0x%02x\n",
> +				 status_entry->reg_iir);

Ratelimited.

> +		}
> +
> +		usb_serial_debug_data(&port->dev, __func__, status_len,
> +				      urb->transfer_buffer + i);
> +
> +		i += status_len;
> +	}
> +
> +exit:
> +	ret = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (ret)
> +		dev_err(&urb->dev->dev, "%s - usb_submit_urb failed; %d\n",

As mentioned above, please use a unified format for error message
without __func__, for example:

	"failed to resubmit status urb: %d\n"

> +			__func__, ret);
> +}
> +
> +/*
> + * Some values came from vendor tree, and we have no meaning for them, this
> + * function simply use them.
> + */
> +static int ch348_do_magic(struct ch348 *ch348, int portnum, u8 action, u8 reg, u8 control)
> +{
> +	struct ch348_magic *buffer;
> +	int ret, len;
> +
> +	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	if (portnum < 4)
> +		reg += 0x10 * portnum;
> +	else
> +		reg += 0x10 * (portnum - 4) + 0x08;
> +
> +	buffer->action = action;
> +	buffer->reg = reg;
> +	buffer->control = control;
> +
> +	ret = usb_bulk_msg(ch348->udev, ch348->cmd_ep, buffer, 3, &len,
> +			   CH348_CMD_TIMEOUT);
> +	if (ret)
> +		dev_err(&ch348->udev->dev, "Failed to write magic err=%d\n", ret);

As above: "failed to write magic: %d\n"

> +
> +	kfree(buffer);
> +
> +	return ret;
> +}
> +
> +static int ch348_configure(struct ch348 *ch348, int portnum)
> +{
> +	int ret;
> +
> +	ret = ch348_do_magic(ch348, portnum, CMD_W_R, R_C2, 0x87);

This looks like it could be a write to the standard Fifo Control
Register.

> +	if (ret)
> +		return ret;
> +
> +	return ch348_do_magic(ch348, portnum, CMD_W_R, R_C4, 0x08);
> +}
> +
> +static void ch348_process_read_urb(struct urb *urb)
> +{
> +	struct usb_serial_port *port = urb->context;
> +	struct ch348 *ch348 = usb_get_serial_data(port->serial);
> +	unsigned int portnum, usblen, i;
> +	struct ch348_rxbuf *rxb;
> +
> +	if (urb->actual_length < 2) {
> +		dev_dbg(&ch348->udev->dev, "Empty rx buffer\n");
> +		return;
> +	}
> +
> +	for (i = 0; i < urb->actual_length; i += CH348_RX_PORT_CHUNK_LENGTH) {
> +		rxb = urb->transfer_buffer + i;
> +		portnum = rxb->port;
> +		if (portnum >= CH348_MAXPORT) {
> +			dev_dbg(&ch348->udev->dev, "Invalid port %d\n", portnum);
> +			break;
> +		}
> +
> +		port = ch348->serial->port[portnum];
> +
> +		usblen = rxb->length;
> +		if (usblen > CH348_RX_PORT_MAX_LENGTH) {
> +			dev_dbg(&port->dev, "Invalid length %d for port %d\n",
> +				usblen, portnum);
> +			break;
> +		}
> +
> +		tty_insert_flip_string(&port->port, rxb->data, usblen);
> +		tty_flip_buffer_push(&port->port);
> +		port->icount.rx += usblen;
> +		usb_serial_debug_data(&port->dev, __func__, usblen, rxb->data);
> +	}
> +}
> +
> +static int ch348_prepare_write_buffer(struct usb_serial_port *port, void *dest, size_t size)
> +{
> +	struct ch348_txbuf *rxt = dest;
> +	int count;
> +
> +	count = kfifo_out_locked(&port->write_fifo, rxt->data,
> +				 size - CH348_TX_HDRSIZE, &port->lock);
> +
> +	rxt->port = port->port_number;
> +	rxt->length = cpu_to_le16(count);
> +
> +	return count + CH348_TX_HDRSIZE;
> +}
> +
> +static int ch348_write(struct tty_struct *tty, struct usb_serial_port *port,
> +		       const unsigned char *buf, int count)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(port->serial);
> +	struct ch348_port *ch348_port = &ch348->ports[port->port_number];
> +	int ret, max_tx_size;
> +
> +	if (tty_get_baud_rate(tty) < 9600 && count >= 128)

You don't hold the termios lock here so this needs to be handled
differently. Perhaps you can set a flag in set_termios if this is
really needed.

> +		/*
> +		 * Writing larger buffers can take longer than the hardware
> +		 * allows before discarding the write buffer. Limit the
> +		 * transfer size in such cases.
> +		 * These values have been found by empirical testing.
> +		 */
> +		max_tx_size = 128;

Can you elaborate on you findings here? According to the vendor homepage
this device has a 1024 byte TX fifo per port. Are you really saying that
for some reason you can only fill it with 128 b when the line speed is
below 9600?

> +	else
> +		/*
> +		* Only ingest as many bytes as we can transfer with one URB at
> +		* a time. Once an URB has been written we need to wait for the
> +		* R_II_B2 status event before we are allowed to send more data.

As I mentioned above R_II_B2 appears to be the transfer holding register
empty flag in IIR. So you write one endpoint size worth of data and then
wait for all of it to be processed on the device before sending more.

How big is the endpoint (please post lsusb -v)?

> +		* If we ingest more data then usb_serial_generic_write() will
> +		* internally try to process as much data as possible with any
> +		* number of URBs without giving us the chance to wait in
> +		* between transfers.

If the hardware really works this way, then perhaps you should not use
the generic write implementation. Just maintain a single urb per port
and don't submit it until the device fifo is empty.

> +		*/
> +		max_tx_size = port->bulk_out_size - CH348_TX_HDRSIZE;
> +
> +	reinit_completion(&ch348_port->write_completion);

This is broken as write can be called at any time and may clear any
previous completion.

> +
> +	mutex_lock(&ch348->write_lock);

Write should not be blocking. Just queue data in the fifo and start a
new transfer when the hardware allows you to.

> +
> +	/*
> +	 * For any (remaining) bytes that we did not transfer TTY core will
> +	 * call us again, with the buffer and count adjusted to the remaining
> +	 * data.
> +	 */
> +	ret = usb_serial_generic_write(tty, port, buf, min(count, max_tx_size));
> +
> +	mutex_unlock(&ch348->write_lock);
> +
> +	if (ret <= 0)
> +		return ret;
> +
> +	if (!wait_for_completion_interruptible_timeout(&ch348_port->write_completion,
> +						       msecs_to_jiffies(CH348_CMD_TIMEOUT))) {
> +		dev_err_console(port, "Failed to wait for TX buffer flush\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return ret;
> +}
> +
> +static int ch348_set_uartmode(struct ch348 *ch348, int portnum, u8 mode)
> +{
> +	int ret;
> +
> +	if (ch348->ports[portnum].uartmode == M_NOR && mode == M_HF) {
> +		ret = ch348_do_magic(ch348, portnum, CMD_W_BR, R_C4, 0x51);
> +		if (ret)
> +			return ret;
> +		ch348->ports[portnum].uartmode = M_HF;
> +	}
> +
> +	if (ch348->ports[portnum].uartmode == M_HF && mode == M_NOR) {
> +		ret = ch348_do_magic(ch348, portnum, CMD_W_BR, R_C4, 0x50);
> +		if (ret)
> +			return ret;
> +		ch348->ports[portnum].uartmode = M_NOR;
> +	}
> +	return 0;
> +}
> +
> +static void ch348_set_termios(struct tty_struct *tty, struct usb_serial_port *port,
> +			      const struct ktermios *termios_old)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(port->serial);
> +	struct ktermios *termios = &tty->termios;
> +	int ret, portnum = port->port_number;
> +	struct ch348_initbuf *buffer;
> +	speed_t	baudrate;
> +	u8 format;
> +
> +	if (termios_old && !tty_termios_hw_change(&tty->termios, termios_old))
> +		return;
> +
> +	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> +	if (!buffer) {
> +		if (termios_old)
> +			tty->termios = *termios_old;
> +		return;
> +	}
> +
> +	/*
> +	 * The datasheet states that only baud rates in range of 1200..6000000
> +	 * are supported. Tests however show that even baud rates as low as 50
> +	 * and as high as 12000000 are working in practice.
> +	 */
> +	baudrate = clamp(tty_get_baud_rate(tty), 50, 12000000);

You should update termios with the new rate.

> +
> +	format = termios->c_cflag & CSTOPB ? 2 : 1;

Please use defines for the constants.

> +
> +	buffer->paritytype = 0;
> +	if (termios->c_cflag & PARENB) {
> +		if (termios->c_cflag & PARODD)
> +			buffer->paritytype += 1;
> +		else
> +			buffer->paritytype += 2;
> +		if  (termios->c_cflag & CMSPAR)
> +			buffer->paritytype += 2;
> +	}

Just spell this out all the cases instead of doing this constant
arithmetic and add defines for the parity modes.

> +
> +	switch (C_CSIZE(tty)) {
> +	case CS5:
> +		buffer->databits = 5;
> +		break;
> +	case CS6:
> +		buffer->databits = 6;
> +		break;
> +	case CS7:
> +		buffer->databits = 7;
> +		break;
> +	case CS8:
> +	default:
> +		buffer->databits = 8;
> +		break;
> +	}
> +	buffer->cmd = CMD_WB_E | (portnum & 0x0F);
> +	buffer->reg = R_INIT;
> +	buffer->port = portnum;
> +	buffer->baudrate = cpu_to_be32(baudrate);
> +
> +	if (format == 2)
> +		buffer->format = 0x02;
> +	else if (format == 1)
> +		buffer->format = 0x00;

Eh, format is set to 1 or 2 above, and here you map this to 2 and 0 for
some reason. Use defines here too.

> +
> +	buffer->rate = max_t(speed_t, 5, (10000 * 15 / baudrate) + 1);
> +
> +	ret = usb_bulk_msg(ch348->udev, ch348->cmd_ep, buffer,
> +			   sizeof(*buffer), NULL, CH348_CMD_TIMEOUT);
> +	if (ret < 0) {
> +		dev_err(&ch348->udev->dev, "Failed to change line settings: err=%d\n",
> +			ret);

drop err= in favour of : here and elsewhere.

> +		goto out;
> +	}
> +
> +	ret = ch348_do_magic(ch348, portnum, CMD_W_R, R_C1, 0x0F);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (C_CRTSCTS(tty))
> +		ret = ch348_set_uartmode(ch348, portnum, M_HF);
> +	else
> +		ret = ch348_set_uartmode(ch348, portnum, M_NOR);
> +
> +out:
> +	kfree(buffer);
> +}
> +
> +static int ch348_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(port->serial);
> +	int ret;
> +
> +	if (tty)
> +		ch348_set_termios(tty, port, NULL);
> +
> +	ret = ch348_configure(ch348, port->port_number);
> +	if (ret) {
> +		dev_err(&ch348->udev->dev, "Fail to configure err=%d\n", ret);

"failed to configure: %d\n", and similar elsewhere

> +		return ret;
> +	}
> +
> +	return usb_serial_generic_open(tty, port);
> +}
> +
> +static int ch348_attach(struct usb_serial *serial)
> +{
> +	struct usb_endpoint_descriptor *epcmd, *epstatus;
> +	struct usb_serial_port *port0 = serial->port[1];

Odd variable name for the second port. I guess you really should be
using port[0] here.

> +	struct usb_device *usb_dev = serial->dev;
> +	int status_buffer_size, i, ret;

Don't mix declarations of unrelated variables like this.

> +	struct usb_interface *intf;
> +	struct ch348 *ch348;
> +
> +	intf = usb_ifnum_to_if(usb_dev, 0);

I assume you only have one interface and want to use serial->interface
here.

> +	epstatus = &intf->cur_altsetting->endpoint[2].desc;
> +	epcmd = &intf->cur_altsetting->endpoint[3].desc;

You need to make sure these endpoints exists as well (e.g. by setting
num_bulk_in/out in the driver data below).

> +
> +	status_buffer_size = usb_endpoint_maxp(epstatus);
> +
> +	ch348 = kzalloc(struct_size(ch348, status_buffer, status_buffer_size),
> +			GFP_KERNEL);
> +	if (!ch348)
> +		return -ENOMEM;
> +
> +	usb_set_serial_data(serial, ch348);
> +
> +	ch348->udev = serial->dev;
> +	ch348->serial = serial;
> +	mutex_init(&ch348->write_lock);
> +
> +	for (i = 0; i < CH348_MAXPORT; i++)
> +		init_completion(&ch348->ports[i].write_completion);
> +
> +	ch348->status_urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!ch348->status_urb) {
> +		ret = -ENOMEM;
> +		goto err_free_ch348;
> +	}
> +
> +	usb_fill_bulk_urb(ch348->status_urb, ch348->udev,
> +			  usb_rcvbulkpipe(ch348->udev, epstatus->bEndpointAddress),
> +			  ch348->status_buffer, status_buffer_size,
> +			  ch348_process_status_urb, ch348);
> +
> +	ret = usb_submit_urb(ch348->status_urb, GFP_KERNEL);
> +	if (ret) {
> +		dev_err(&ch348->udev->dev,
> +			"%s - failed to submit status/interrupt urb %i\n",
> +			__func__, ret);
> +		goto err_free_status_urb;
> +	}
> +
> +	ret = usb_serial_generic_submit_read_urbs(port0, GFP_KERNEL);
> +	if (ret)
> +		goto err_kill_status_urb;

I know we have other driver submitting these at attach() but this really
should be done on first open to avoid wasting resources. I guess this is
ok for now.

> +
> +	ch348->cmd_ep = usb_sndbulkpipe(usb_dev, epcmd->bEndpointAddress);
> +
> +	return 0;
> +
> +err_kill_status_urb:
> +	usb_kill_urb(ch348->status_urb);
> +err_free_status_urb:
> +	usb_free_urb(ch348->status_urb);
> +err_free_ch348:
> +	kfree(ch348);
> +	return ret;
> +}
> +
> +static void ch348_release(struct usb_serial *serial)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(serial);
> +
> +	usb_kill_urb(ch348->status_urb);
> +	usb_free_urb(ch348->status_urb);
> +
> +	kfree(ch348);
> +}
> +
> +static void ch348_print_version(struct usb_serial *serial)
> +{
> +	u8 *version_buf;
> +	int ret;
> +
> +	version_buf = kzalloc(4, GFP_KERNEL);
> +	if (!version_buf)
> +		return;
> +
> +	ret = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> +			      CMD_VER,
> +			      USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> +			      0, 0, version_buf, 4, CH348_CMD_TIMEOUT);
> +	if (ret < 0)
> +		dev_dbg(&serial->dev->dev, "Failed to read CMD_VER: %d\n", ret);
> +	else
> +		dev_info(&serial->dev->dev, "Found WCH CH348%s\n",
> +			 (version_buf[1] & 0x80) ? "Q" : "L");
> +
> +	kfree(version_buf);
> +}
> +
> +static int ch348_probe(struct usb_serial *serial, const struct usb_device_id *id)
> +{
> +	struct usb_endpoint_descriptor *epread, *epwrite, *epstatus, *epcmd;
> +	struct usb_device *usb_dev = serial->dev;
> +	struct usb_interface *intf;
> +	int ret;
> +
> +	intf = usb_ifnum_to_if(usb_dev, 0);

serial->interface

> +
> +	ret = usb_find_common_endpoints(intf->cur_altsetting, &epread, &epwrite,
> +					NULL, NULL);
> +	if (ret) {
> +		dev_err(&serial->dev->dev, "Failed to find basic endpoints ret=%d\n", ret);
> +		return ret;
> +	}

Not needed since you set num_bulk_in/out below (both should be 2 however).

> +	epstatus = &intf->cur_altsetting->endpoint[2].desc;

This could be accessing data beyond the endpoint array currently.

> +	if (!usb_endpoint_is_bulk_in(epstatus)) {
> +		dev_err(&serial->dev->dev, "Missing second bulk in (STATUS/INT)\n");
> +		return -ENODEV;
> +	}
> +
> +	epcmd = &intf->cur_altsetting->endpoint[3].desc;
> +	if (!usb_endpoint_is_bulk_out(epcmd)) {
> +		dev_err(&serial->dev->dev, "Missing second bulk out (CMD)\n");
> +		return -ENODEV;
> +	}

And then can use just verify the status/cmd direction in attach().

Or you can use usb_check_bulk_endpoints() here.

> +
> +	ch348_print_version(serial);
> +
> +	return 0;
> +}
> +
> +static int ch348_calc_num_ports(struct usb_serial *serial,
> +				struct usb_serial_endpoints *epds)
> +{
> +	int i;
> +
> +	for (i = 1; i < CH348_MAXPORT; ++i) {
> +		epds->bulk_out[i] = epds->bulk_out[0];
> +		epds->bulk_in[i] = epds->bulk_in[0];

No need to set bulk_in[i], all reading is done via the first port.

> +	}
> +
> +	epds->num_bulk_out = CH348_MAXPORT;
> +	epds->num_bulk_in = CH348_MAXPORT;

And then only set num_bulk_out here.

> +
> +	return CH348_MAXPORT;
> +}
> +
> +static int ch348_suspend(struct usb_serial *serial, pm_message_t message)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(serial);
> +
> +	usb_kill_urb(ch348->status_urb);
> +
> +	return 0;
> +}
> +
> +static int ch348_resume(struct usb_serial *serial)
> +{
> +	struct ch348 *ch348 = usb_get_serial_data(serial);
> +	int ret;
> +
> +	ret = usb_submit_urb(ch348->status_urb, GFP_KERNEL);
> +	if (ret) {
> +		dev_err(&ch348->udev->dev,
> +			"%s - failed to submit status/interrupt urb %i\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
> +	ret = usb_serial_generic_resume(serial);
> +	if (ret)
> +		usb_kill_urb(ch348->status_urb);
> +
> +	return ret;
> +}
> +
> +static const struct usb_device_id ch348_ids[] = {
> +	{ USB_DEVICE(0x1a86, 0x55d9), },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(usb, ch348_ids);
> +
> +static struct usb_serial_driver ch348_device = {
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.name = "ch348",
> +	},
> +	.id_table =		ch348_ids,
> +	.num_ports =		CH348_MAXPORT,
> +	.num_bulk_in =		1,
> +	.num_bulk_out =		1,

Set both of these to 2 so that core verifies that you have all four
endpoints.

> +	.open =			ch348_open,
> +	.set_termios =		ch348_set_termios,
> +	.process_read_urb =	ch348_process_read_urb,
> +	.prepare_write_buffer =	ch348_prepare_write_buffer,
> +	.write =		ch348_write,
> +	.probe =		ch348_probe,
> +	.calc_num_ports =	ch348_calc_num_ports,
> +	.attach =		ch348_attach,
> +	.release =		ch348_release,
> +	.suspend =		ch348_suspend,
> +	.resume =		ch348_resume,
> +};
> +
> +static struct usb_serial_driver * const serial_drivers[] = {
> +	&ch348_device, NULL
> +};
> +
> +module_usb_serial_driver(serial_drivers, ch348_ids);
> +
> +MODULE_AUTHOR("Corentin Labbe <clabbe@baylibre.com>");
> +MODULE_DESCRIPTION("USB CH348 Octo port serial converter driver");
> +MODULE_LICENSE("GPL");

Johan

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[Martin Blumenstingl]

Hello Johan,

thanks for taking the time to go through this patch and providing feedback!

On Mon, Jul 22, 2024 at 4:21 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Tue, May 07, 2024 at 01:15:22PM +0000, Corentin Labbe wrote:
> > The CH348 is an USB octo port serial adapter.
> > The device multiplexes all 8 ports in the same pair of Bulk endpoints.
> > Since there is no public datasheet, unfortunately it remains some magic values
>
> Could you please include a pointer to the vendor driver (which I assumed
> you based this on)?
We can do that, in case you want/need some info upfront you can find
the reference code here: [0]

[...]
> > @@ -0,0 +1,725 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * USB serial driver for USB to Octal UARTs chip ch348.
> > + *
> > + * Copyright (C) 2022 Corentin Labbe <clabbe@baylibre.com>
>
> Do you need to include any copyrights from the vendor driver? It looks
> like at least some of the code below was copied from somewhere (but
> perhaps not that much).
If you - or someone else - have any advice on this I'd be happy to go with that!

[...]
> > +#define R_II_B1              0x06
> > +#define R_II_B2              0x02
> > +#define R_II_B3              0x00
>
> This look like standard IIR masks (e.g. R_II_B2 would be Transmit
> holding register empty).
I didn't know about it until now - thanks for the hint.
note to self: git grep UART_IIR_ include/

[...]
> > +     struct urb *status_urb;
> > +     u8 status_buffer[];
>
> This buffer should be allocated separately as it is used for DMA.
I assume it's because of alignment reasons.
Lucky me that I didn't hit any issues on ARM or x86_64.

[...}
> > +     u8 reg_iir;
> > +     union {
> > +             u8 lsr_signal;
> > +             u8 modem_signal;
> > +             u8 init_data[10];
>
> This init_data field is never used and looks a bit odd. What is it used
> for?
I think the firmware is sending back the configuration that was
applied (see ch348_set_termios())

> You mentioned that that the modem lines are not yet used either.
My understanding is: we neither use init_data nor modem_signal in the
driver so let's drop them for now (and add them back when they're
actually needed).

[...]
> > +                     if (status_entry->lsr_signal & CH348_LO)
> > +                             port->icount.overrun++;
> > +                     if (status_entry->lsr_signal & CH348_LP)
> > +                             port->icount.parity++;
> > +                     if (status_entry->lsr_signal & CH348_LF)
> > +                             port->icount.frame++;
> > +                     if (status_entry->lsr_signal & CH348_LF)
>
> Should you really count every framing error as a break? Looks like this
> should have been CH348_LB.
Excellent catch - thank you!

Also note to self: git grep UART_LSR_ include
We can probably replace the custom #defines with the standard ones.

[...]
> > +static int ch348_configure(struct ch348 *ch348, int portnum)
> > +{
> > +     int ret;
> > +
> > +     ret = ch348_do_magic(ch348, portnum, CMD_W_R, R_C2, 0x87);
>
> This looks like it could be a write to the standard Fifo Control
> Register.
And again, thank you! 0x87 translates to (which also makes sense):
  UART_FCR_ENABLE_FIFO (0x01) |
  UART_FCR_CLEAR_RCVR (0x02) |
  UART_FCR_CLEAR_XMIT (0x04) |
  UART_FCR_R_TRIG_10 (0x80) |
  UART_FCR_T_TRIG_00 (0x00)

[...]
> > +static int ch348_write(struct tty_struct *tty, struct usb_serial_port *port,
> > +                    const unsigned char *buf, int count)
> > +{
> > +     struct ch348 *ch348 = usb_get_serial_data(port->serial);
> > +     struct ch348_port *ch348_port = &ch348->ports[port->port_number];
> > +     int ret, max_tx_size;
> > +
> > +     if (tty_get_baud_rate(tty) < 9600 && count >= 128)
>
> You don't hold the termios lock here so this needs to be handled
> differently. Perhaps you can set a flag in set_termios if this is
> really needed.
>
> > +             /*
> > +              * Writing larger buffers can take longer than the hardware
> > +              * allows before discarding the write buffer. Limit the
> > +              * transfer size in such cases.
> > +              * These values have been found by empirical testing.
> > +              */
> > +             max_tx_size = 128;
>
> Can you elaborate on you findings here? According to the vendor homepage
> this device has a 1024 byte TX fifo per port. Are you really saying that
> for some reason you can only fill it with 128 b when the line speed is
> below 9600?
For slow speeds I never receive the "Transmitter holding register
empty" interrupt/signal when using the full TX buffer.
It's not that the interrupt/signal is late - it just never arrives.
I don't know why that is (whether the firmware tries to keep things
"fair" for other ports, ...) though.

> > +     else
> > +             /*
> > +             * Only ingest as many bytes as we can transfer with one URB at
> > +             * a time. Once an URB has been written we need to wait for the
> > +             * R_II_B2 status event before we are allowed to send more data.
>
> As I mentioned above R_II_B2 appears to be the transfer holding register
> empty flag in IIR. So you write one endpoint size worth of data and then
> wait for all of it to be processed on the device before sending more.
>
> How big is the endpoint (please post lsusb -v)?
Bus 003 Device 022: ID 1a86:55d9 QinHeng Electronics USB2.0 To Multi
Serial Ports
Device Descriptor:
 bLength                18
 bDescriptorType         1
 bcdUSB               2.00
 bDeviceClass          255 Vendor Specific Class
 bDeviceSubClass       128 [unknown]
 bDeviceProtocol        55
 bMaxPacketSize0        64
 idVendor           0x1a86 QinHeng Electronics
 idProduct          0x55d9 USB2.0 To Multi Serial Ports
 bcdDevice            1.36
 iManufacturer           1 wch.cn
 iProduct                2 USB2.0 To Multi Serial Ports
 iSerial                 0
 bNumConfigurations      1
 Configuration Descriptor:
   bLength                 9
   bDescriptorType         2
   wTotalLength       0x002e
   bNumInterfaces          1
   bConfigurationValue     1
   iConfiguration          0
   bmAttributes         0x80
     (Bus Powered)
   MaxPower              200mA
   Interface Descriptor:
     bLength                 9
     bDescriptorType         4
     bInterfaceNumber        0
     bAlternateSetting       0
     bNumEndpoints           4
     bInterfaceClass       255 Vendor Specific Class
     bInterfaceSubClass    128 [unknown]
     bInterfaceProtocol     55
     iInterface              0
     Endpoint Descriptor:
       bLength                 7
       bDescriptorType         5
       bEndpointAddress     0x82  EP 2 IN
       bmAttributes            2
         Transfer Type            Bulk
         Synch Type               None
         Usage Type               Data
       wMaxPacketSize     0x0200  1x 512 bytes
       bInterval               0
     Endpoint Descriptor:
       bLength                 7
       bDescriptorType         5
       bEndpointAddress     0x02  EP 2 OUT
       bmAttributes            2
         Transfer Type            Bulk
         Synch Type               None
         Usage Type               Data
       wMaxPacketSize     0x0200  1x 512 bytes
       bInterval               0
     Endpoint Descriptor:
       bLength                 7
       bDescriptorType         5
       bEndpointAddress     0x81  EP 1 IN
       bmAttributes            2
         Transfer Type            Bulk
         Synch Type               None
         Usage Type               Data
       wMaxPacketSize     0x0200  1x 512 bytes
       bInterval               0
     Endpoint Descriptor:
       bLength                 7
       bDescriptorType         5
       bEndpointAddress     0x01  EP 1 OUT
       bmAttributes            2
         Transfer Type            Bulk
         Synch Type               None
         Usage Type               Data
       wMaxPacketSize     0x0200  1x 512 bytes
       bInterval               0
Device Qualifier (for other device speed):
 bLength                10
 bDescriptorType         6
 bcdUSB               2.00
 bDeviceClass          255 Vendor Specific Class
 bDeviceSubClass         0 [unknown]
 bDeviceProtocol       255
 bMaxPacketSize0        64
 bNumConfigurations      1
can't get debug descriptor: Resource temporarily unavailable
Device Status:     0x0000
 (Bus Powered)

> > +             * If we ingest more data then usb_serial_generic_write() will
> > +             * internally try to process as much data as possible with any
> > +             * number of URBs without giving us the chance to wait in
> > +             * between transfers.
>
> If the hardware really works this way, then perhaps you should not use
> the generic write implementation. Just maintain a single urb per port
> and don't submit it until the device fifo is empty.
I tried to avoid having to copy & paste (which then also means having
to maintain it down the line) most of the generic write
implementation.
This whole dance with waiting for the "Transmitter holding register
empty" by the way was the reason why parts of the transmit buffer got
lost, see the report from Nicolas in v6 [1]

> > +             */
> > +             max_tx_size = port->bulk_out_size - CH348_TX_HDRSIZE;
> > +
> > +     reinit_completion(&ch348_port->write_completion);
>
> This is broken as write can be called at any time and may clear any
> previous completion.
I never hit that issue in my tests but I get your point.

[...]
> > +static struct usb_serial_driver ch348_device = {
> > +     .driver = {
> > +             .owner = THIS_MODULE,
> > +             .name = "ch348",
> > +     },
> > +     .id_table =             ch348_ids,
> > +     .num_ports =            CH348_MAXPORT,
> > +     .num_bulk_in =          1,
> > +     .num_bulk_out =         1,
>
> Set both of these to 2 so that core verifies that you have all four
> endpoints.
I will have to test this because I thought that:
- using 2 here makes usb-serial allocate an URB as well and by default
assign it to the first and second port
- usb-serial should not touch the second bulk in / bulk out endpoint
(as they're entirely vendor / chip specific)

I omitted replies to some of your comments because I agree with them
and repeatedly writing "ACK" didn't seem to add much value.


Best regards,
Martin


[0] https://github.com/WCHSoftGroup/ch9344ser_linux/tree/dcbe60d12ead4622b7fdd6dec24e62facee23663/driver
[1] https://lore.kernel.org/lkml/2595072.9XhBIDAVAK@archbook/

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[Johan Hovold]

On Mon, Jul 22, 2024 at 11:22:32PM +0200, Martin Blumenstingl wrote:

> On Mon, Jul 22, 2024 at 4:21 PM Johan Hovold <johan@kernel.org> wrote:
> > On Tue, May 07, 2024 at 01:15:22PM +0000, Corentin Labbe wrote:

> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * USB serial driver for USB to Octal UARTs chip ch348.
> > > + *
> > > + * Copyright (C) 2022 Corentin Labbe <clabbe@baylibre.com>
> >
> > Do you need to include any copyrights from the vendor driver? It looks
> > like at least some of the code below was copied from somewhere (but
> > perhaps not that much).

> If you - or someone else - have any advice on this I'd be happy to go with that!

If you copy code directly (even if you clean it up after) you should
include it, but not necessarily if you just use it for reference for the
protocol.

It doesn't hurt mentioning anyway when you add the reference to the
vendor driver, for example:

	Based on the XXX driver:

		https://...

	Copyright YYY

> > > +     u8 reg_iir;
> > > +     union {
> > > +             u8 lsr_signal;
> > > +             u8 modem_signal;
> > > +             u8 init_data[10];
> >
> > This init_data field is never used and looks a bit odd. What is it used
> > for?
> I think the firmware is sending back the configuration that was
> applied (see ch348_set_termios())
> 
> > You mentioned that that the modem lines are not yet used either.

> My understanding is: we neither use init_data nor modem_signal in the
> driver so let's drop them for now (and add them back when they're
> actually needed).

I think it's ok to keep, as reference. You do dump this data later even
if you don't currently use this union I noticed.

> > > +static int ch348_write(struct tty_struct *tty, struct usb_serial_port *port,
> > > +                    const unsigned char *buf, int count)
> > > +{
> > > +     struct ch348 *ch348 = usb_get_serial_data(port->serial);
> > > +     struct ch348_port *ch348_port = &ch348->ports[port->port_number];
> > > +     int ret, max_tx_size;
> > > +
> > > +     if (tty_get_baud_rate(tty) < 9600 && count >= 128)
> >
> > You don't hold the termios lock here so this needs to be handled
> > differently. Perhaps you can set a flag in set_termios if this is
> > really needed.
> >
> > > +             /*
> > > +              * Writing larger buffers can take longer than the hardware
> > > +              * allows before discarding the write buffer. Limit the
> > > +              * transfer size in such cases.
> > > +              * These values have been found by empirical testing.
> > > +              */
> > > +             max_tx_size = 128;
> >
> > Can you elaborate on you findings here? According to the vendor homepage
> > this device has a 1024 byte TX fifo per port. Are you really saying that
> > for some reason you can only fill it with 128 b when the line speed is
> > below 9600?

> For slow speeds I never receive the "Transmitter holding register
> empty" interrupt/signal when using the full TX buffer.
> It's not that the interrupt/signal is late - it just never arrives.
> I don't know why that is (whether the firmware tries to keep things
> "fair" for other ports, ...) though.

Perhaps you can run some isolated experiments if you haven't already.
Submitting just a single URB with say 128, 512 or 1024 bytes of data and
see when/if you ever receive a transmitter holding empty interrupt.

How does the vendor driver handle this? Does it really wait for the THRE
interrupt before submitting more data?

You could try increasing the buffer size to 2k and see how much is
received on the other end if you submit one URB (e.g. does the hardware
just drop the last 1k of data when the device fifo is full).
 
> > > +     else
> > > +             /*
> > > +             * Only ingest as many bytes as we can transfer with one URB at
> > > +             * a time. Once an URB has been written we need to wait for the
> > > +             * R_II_B2 status event before we are allowed to send more data.
> >
> > As I mentioned above R_II_B2 appears to be the transfer holding register
> > empty flag in IIR. So you write one endpoint size worth of data and then
> > wait for all of it to be processed on the device before sending more.
> >
> > How big is the endpoint (please post lsusb -v)?

>        wMaxPacketSize     0x0200  1x 512 bytes

> > > +             * If we ingest more data then usb_serial_generic_write() will
> > > +             * internally try to process as much data as possible with any
> > > +             * number of URBs without giving us the chance to wait in
> > > +             * between transfers.
> >
> > If the hardware really works this way, then perhaps you should not use
> > the generic write implementation. Just maintain a single urb per port
> > and don't submit it until the device fifo is empty.

> I tried to avoid having to copy & paste (which then also means having
> to maintain it down the line) most of the generic write
> implementation.
> This whole dance with waiting for the "Transmitter holding register
> empty" by the way was the reason why parts of the transmit buffer got
> lost, see the report from Nicolas in v6 [1]

I understand, but the generic implementation is not a good fit here as
it actively tries to make sure the device buffers are always full (e.g.
by using two URBs back-to-back).

If you can't find a way to make the hardware behave properly then a
custom implementation using a single URBs is preferable over trying
to limit the generic implementation like you did here. Perhaps bits can
be reused anyway (e.g. chars_in_buffer if you use the write fifo).

> > > +static struct usb_serial_driver ch348_device = {
> > > +     .driver = {
> > > +             .owner = THIS_MODULE,
> > > +             .name = "ch348",
> > > +     },
> > > +     .id_table =             ch348_ids,
> > > +     .num_ports =            CH348_MAXPORT,
> > > +     .num_bulk_in =          1,
> > > +     .num_bulk_out =         1,
> >
> > Set both of these to 2 so that core verifies that you have all four
> > endpoints.

> I will have to test this because I thought that:
> - using 2 here makes usb-serial allocate an URB as well and by default
> assign it to the first and second port
> - usb-serial should not touch the second bulk in / bulk out endpoint
> (as they're entirely vendor / chip specific)

Setting these two should make core make sure that the endpoints exist,
and by default they will be assigned to the first and second port, but
you can override that calc_num_endpoints() (as you already do).

For the second IN EP, you could even let core allocate the URB and use
that instead of doing so manually (e.g. by submitting yourself or using
the generic read implementation as mxuport does).

Johan

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[Martin Blumenstingl]

Hi Johan,

On Tue, Jul 23, 2024 at 6:13 PM Johan Hovold <johan@kernel.org> wrote:
[...]
> > > Do you need to include any copyrights from the vendor driver? It looks
> > > like at least some of the code below was copied from somewhere (but
> > > perhaps not that much).
>
> > If you - or someone else - have any advice on this I'd be happy to go with that!
>
> If you copy code directly (even if you clean it up after) you should
> include it, but not necessarily if you just use it for reference for the
> protocol.
>
> It doesn't hurt mentioning anyway when you add the reference to the
> vendor driver, for example:
>
>         Based on the XXX driver:
>
>                 https://...
>
>         Copyright YYY
Thanks, I'll include that in the next version.

[...]
> > For slow speeds I never receive the "Transmitter holding register
> > empty" interrupt/signal when using the full TX buffer.
> > It's not that the interrupt/signal is late - it just never arrives.
> > I don't know why that is (whether the firmware tries to keep things
> > "fair" for other ports, ...) though.
>
> Perhaps you can run some isolated experiments if you haven't already.
> Submitting just a single URB with say 128, 512 or 1024 bytes of data and
> see when/if you ever receive a transmitter holding empty interrupt.
>
> How does the vendor driver handle this? Does it really wait for the THRE
> interrupt before submitting more data?
The vendor driver:
- first acquires a per-device (not per port) write_lock [0]
- then waits for the (per-device, not per port) write buffer to be empty [1]
- and only then submits more data to be transmitted [2]

> You could try increasing the buffer size to 2k and see how much is
> received on the other end if you submit one URB (e.g. does the hardware
> just drop the last 1k of data when the device fifo is full).
I have not tried this yet but if still relevant (after the info about
the THRE interrupt) then I can try it and share the results.

[...]
> > > > +             * If we ingest more data then usb_serial_generic_write() will
> > > > +             * internally try to process as much data as possible with any
> > > > +             * number of URBs without giving us the chance to wait in
> > > > +             * between transfers.
> > >
> > > If the hardware really works this way, then perhaps you should not use
> > > the generic write implementation. Just maintain a single urb per port
> > > and don't submit it until the device fifo is empty.
>
> > I tried to avoid having to copy & paste (which then also means having
> > to maintain it down the line) most of the generic write
> > implementation.
> > This whole dance with waiting for the "Transmitter holding register
> > empty" by the way was the reason why parts of the transmit buffer got
> > lost, see the report from Nicolas in v6 [1]
>
> I understand, but the generic implementation is not a good fit here as
> it actively tries to make sure the device buffers are always full (e.g.
> by using two URBs back-to-back).
>
> If you can't find a way to make the hardware behave properly then a
> custom implementation using a single URBs is preferable over trying
> to limit the generic implementation like you did here. Perhaps bits can
> be reused anyway (e.g. chars_in_buffer if you use the write fifo).
I cannot find any other usb-serial driver which uses this pattern.
Most devices seem to be happy to take more data once they trigger the
write_bulk_callback but not ch348.

If there's any other (even if it's not a usb-serial) driver that I can
use as a reference implementation for your suggestion?
I'm not sure whether to use a dedicated kthread, single threaded workqueue, ...

> > > > +static struct usb_serial_driver ch348_device = {
> > > > +     .driver = {
> > > > +             .owner = THIS_MODULE,
> > > > +             .name = "ch348",
> > > > +     },
> > > > +     .id_table =             ch348_ids,
> > > > +     .num_ports =            CH348_MAXPORT,
> > > > +     .num_bulk_in =          1,
> > > > +     .num_bulk_out =         1,
> > >
> > > Set both of these to 2 so that core verifies that you have all four
> > > endpoints.
>
> > I will have to test this because I thought that:
> > - using 2 here makes usb-serial allocate an URB as well and by default
> > assign it to the first and second port
> > - usb-serial should not touch the second bulk in / bulk out endpoint
> > (as they're entirely vendor / chip specific)
>
> Setting these two should make core make sure that the endpoints exist,
> and by default they will be assigned to the first and second port, but
> you can override that calc_num_endpoints() (as you already do).
>
> For the second IN EP, you could even let core allocate the URB and use
> that instead of doing so manually (e.g. by submitting yourself or using
> the generic read implementation as mxuport does).
Thanks for the hint - I have tried this and it indeed simplifies the code!


Best regards,
Martin


[0] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1100
[1] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1152-L1156
[2] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1166

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[David Heidelberg]

Hello Martin,

are you planning sending v8 soon?

There seems to be general interest, if it makes working on it more joyful:
https://www.reddit.com/r/raspberry_pi/comments/1gznt7p/has_anyone_used_this_board_usb_to_8channel_serial/

Thank you
David

On 25/08/2024 06:56, Martin Blumenstingl wrote:
> Hi Johan,
> 
> On Tue, Jul 23, 2024 at 6:13 PM Johan Hovold <johan@kernel.org> wrote:
> [...]
>>>> Do you need to include any copyrights from the vendor driver? It looks
>>>> like at least some of the code below was copied from somewhere (but
>>>> perhaps not that much).
>>
>>> If you - or someone else - have any advice on this I'd be happy to go with that!
>>
>> If you copy code directly (even if you clean it up after) you should
>> include it, but not necessarily if you just use it for reference for the
>> protocol.
>>
>> It doesn't hurt mentioning anyway when you add the reference to the
>> vendor driver, for example:
>>
>>          Based on the XXX driver:
>>
>>                  https://...
>>
>>          Copyright YYY
> Thanks, I'll include that in the next version.
> 
> [...]
>>> For slow speeds I never receive the "Transmitter holding register
>>> empty" interrupt/signal when using the full TX buffer.
>>> It's not that the interrupt/signal is late - it just never arrives.
>>> I don't know why that is (whether the firmware tries to keep things
>>> "fair" for other ports, ...) though.
>>
>> Perhaps you can run some isolated experiments if you haven't already.
>> Submitting just a single URB with say 128, 512 or 1024 bytes of data and
>> see when/if you ever receive a transmitter holding empty interrupt.
>>
>> How does the vendor driver handle this? Does it really wait for the THRE
>> interrupt before submitting more data?
> The vendor driver:
> - first acquires a per-device (not per port) write_lock [0]
> - then waits for the (per-device, not per port) write buffer to be empty [1]
> - and only then submits more data to be transmitted [2]
> 
>> You could try increasing the buffer size to 2k and see how much is
>> received on the other end if you submit one URB (e.g. does the hardware
>> just drop the last 1k of data when the device fifo is full).
> I have not tried this yet but if still relevant (after the info about
> the THRE interrupt) then I can try it and share the results.
> 
> [...]
>>>>> +             * If we ingest more data then usb_serial_generic_write() will
>>>>> +             * internally try to process as much data as possible with any
>>>>> +             * number of URBs without giving us the chance to wait in
>>>>> +             * between transfers.
>>>>
>>>> If the hardware really works this way, then perhaps you should not use
>>>> the generic write implementation. Just maintain a single urb per port
>>>> and don't submit it until the device fifo is empty.
>>
>>> I tried to avoid having to copy & paste (which then also means having
>>> to maintain it down the line) most of the generic write
>>> implementation.
>>> This whole dance with waiting for the "Transmitter holding register
>>> empty" by the way was the reason why parts of the transmit buffer got
>>> lost, see the report from Nicolas in v6 [1]
>>
>> I understand, but the generic implementation is not a good fit here as
>> it actively tries to make sure the device buffers are always full (e.g.
>> by using two URBs back-to-back).
>>
>> If you can't find a way to make the hardware behave properly then a
>> custom implementation using a single URBs is preferable over trying
>> to limit the generic implementation like you did here. Perhaps bits can
>> be reused anyway (e.g. chars_in_buffer if you use the write fifo).
> I cannot find any other usb-serial driver which uses this pattern.
> Most devices seem to be happy to take more data once they trigger the
> write_bulk_callback but not ch348.
> 
> If there's any other (even if it's not a usb-serial) driver that I can
> use as a reference implementation for your suggestion?
> I'm not sure whether to use a dedicated kthread, single threaded workqueue, ...
> 
>>>>> +static struct usb_serial_driver ch348_device = {
>>>>> +     .driver = {
>>>>> +             .owner = THIS_MODULE,
>>>>> +             .name = "ch348",
>>>>> +     },
>>>>> +     .id_table =             ch348_ids,
>>>>> +     .num_ports =            CH348_MAXPORT,
>>>>> +     .num_bulk_in =          1,
>>>>> +     .num_bulk_out =         1,
>>>>
>>>> Set both of these to 2 so that core verifies that you have all four
>>>> endpoints.
>>
>>> I will have to test this because I thought that:
>>> - using 2 here makes usb-serial allocate an URB as well and by default
>>> assign it to the first and second port
>>> - usb-serial should not touch the second bulk in / bulk out endpoint
>>> (as they're entirely vendor / chip specific)
>>
>> Setting these two should make core make sure that the endpoints exist,
>> and by default they will be assigned to the first and second port, but
>> you can override that calc_num_endpoints() (as you already do).
>>
>> For the second IN EP, you could even let core allocate the URB and use
>> that instead of doing so manually (e.g. by submitting yourself or using
>> the generic read implementation as mxuport does).
> Thanks for the hint - I have tried this and it indeed simplifies the code!
> 
> 
> Best regards,
> Martin
> 
> 
> [0] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1100
> [1] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1152-L1156
> [2] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1166

-- 
David Heidelberg

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[Corentin LABBE]

Le Fri, Nov 29, 2024 at 09:52:24PM -0500, David Heidelberg a écrit :
> Hello Martin,
> 
> are you planning sending v8 soon?
> 
> There seems to be general interest, if it makes working on it more joyful:
> https://www.reddit.com/r/raspberry_pi/comments/1gznt7p/has_anyone_used_this_board_usb_to_8channel_serial/
> 
> Thank you
> David
> 

Hello

Holidays give me finaly the time to rebuild my testing setup.
V8 is tested and no obvious problem is found.

So I will send v8 early this week.

Regards

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[Corentin LABBE]

Le Fri, Nov 29, 2024 at 09:52:24PM -0500, David Heidelberg a écrit :
> Hello Martin,
> 
> are you planning sending v8 soon?
> 
> There seems to be general interest, if it makes working on it more joyful:
> https://www.reddit.com/r/raspberry_pi/comments/1gznt7p/has_anyone_used_this_board_usb_to_8channel_serial/
> 
> Thank you
> David
> 

Hello

v8 is delayed, my testsuite fail on linux-next only.
I bisected to a faulty commit "libfs: Use d_children list to iterate simple_offset directorie" but I dont understand ho it can cause proble on serial....

Regards

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[Johan Hovold]

Hi Martin (and Corentin),

and sorry about the late reply here.

On Sun, Aug 25, 2024 at 12:56:17PM +0200, Martin Blumenstingl wrote:
> On Tue, Jul 23, 2024 at 6:13 PM Johan Hovold <johan@kernel.org> wrote:

> > > For slow speeds I never receive the "Transmitter holding register
> > > empty" interrupt/signal when using the full TX buffer.
> > > It's not that the interrupt/signal is late - it just never arrives.
> > > I don't know why that is (whether the firmware tries to keep things
> > > "fair" for other ports, ...) though.
> >
> > Perhaps you can run some isolated experiments if you haven't already.
> > Submitting just a single URB with say 128, 512 or 1024 bytes of data and
> > see when/if you ever receive a transmitter holding empty interrupt.
> >
> > How does the vendor driver handle this? Does it really wait for the THRE
> > interrupt before submitting more data?

> The vendor driver:
> - first acquires a per-device (not per port) write_lock [0]
> - then waits for the (per-device, not per port) write buffer to be empty [1]
> - and only then submits more data to be transmitted [2]

Ok, thanks for confirming, sounds like that's how the device works then.

> > You could try increasing the buffer size to 2k and see how much is
> > received on the other end if you submit one URB (e.g. does the hardware
> > just drop the last 1k of data when the device fifo is full).

> I have not tried this yet but if still relevant (after the info about
> the THRE interrupt) then I can try it and share the results.

It would only be to really confirm that this is how the vendor protocol
and device works. Your call.

> [...]
> > > > > +             * If we ingest more data then usb_serial_generic_write() will
> > > > > +             * internally try to process as much data as possible with any
> > > > > +             * number of URBs without giving us the chance to wait in
> > > > > +             * between transfers.
> > > >
> > > > If the hardware really works this way, then perhaps you should not use
> > > > the generic write implementation. Just maintain a single urb per port
> > > > and don't submit it until the device fifo is empty.
> >
> > > I tried to avoid having to copy & paste (which then also means having
> > > to maintain it down the line) most of the generic write
> > > implementation.
> > > This whole dance with waiting for the "Transmitter holding register
> > > empty" by the way was the reason why parts of the transmit buffer got
> > > lost, see the report from Nicolas in v6 [1]
> >
> > I understand, but the generic implementation is not a good fit here as
> > it actively tries to make sure the device buffers are always full (e.g.
> > by using two URBs back-to-back).
> >
> > If you can't find a way to make the hardware behave properly then a
> > custom implementation using a single URBs is preferable over trying
> > to limit the generic implementation like you did here. Perhaps bits can
> > be reused anyway (e.g. chars_in_buffer if you use the write fifo).

> I cannot find any other usb-serial driver which uses this pattern.
> Most devices seem to be happy to take more data once they trigger the
> write_bulk_callback but not ch348.

Right. I think the io_edgeport driver maintains some kind of TxCredits.
I guess that's related, but not sure how relevant that is here (and you
probably shouldn't based anything on that old driver directly anyway).

> If there's any other (even if it's not a usb-serial) driver that I can
> use as a reference implementation for your suggestion?
> I'm not sure whether to use a dedicated kthread, single threaded workqueue, ...

Not sure what Corentin has been preparing, but yeah, you need some kind
of deferred mechanism to make write() non-blocking and hold off sending
more data to the device until you're sure there's room in its buffers. I
guess a workqueue should do fine.

Johan

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[Martin Blumenstingl]

Hi Johan,

On Tue, Jan 14, 2025 at 4:39 PM Johan Hovold <johan@kernel.org> wrote:
>
> Hi Martin (and Corentin),
>
> and sorry about the late reply here.
Sometimes there's more important things in life than USB to serial adapters.

[...]
> > > You could try increasing the buffer size to 2k and see how much is
> > > received on the other end if you submit one URB (e.g. does the hardware
> > > just drop the last 1k of data when the device fifo is full).
>
> > I have not tried this yet but if still relevant (after the info about
> > the THRE interrupt) then I can try it and share the results.
>
> It would only be to really confirm that this is how the vendor protocol
> and device works. Your call.
I can give that a try in the next few days and will report back.

[...]
> > > I understand, but the generic implementation is not a good fit here as
> > > it actively tries to make sure the device buffers are always full (e.g.
> > > by using two URBs back-to-back).
> > >
> > > If you can't find a way to make the hardware behave properly then a
> > > custom implementation using a single URBs is preferable over trying
> > > to limit the generic implementation like you did here. Perhaps bits can
> > > be reused anyway (e.g. chars_in_buffer if you use the write fifo).
>
> > I cannot find any other usb-serial driver which uses this pattern.
> > Most devices seem to be happy to take more data once they trigger the
> > write_bulk_callback but not ch348.
>
> Right. I think the io_edgeport driver maintains some kind of TxCredits.
> I guess that's related, but not sure how relevant that is here (and you
> probably shouldn't based anything on that old driver directly anyway).
>
> > If there's any other (even if it's not a usb-serial) driver that I can
> > use as a reference implementation for your suggestion?
> > I'm not sure whether to use a dedicated kthread, single threaded workqueue, ...
>
> Not sure what Corentin has been preparing, but yeah, you need some kind
> of deferred mechanism to make write() non-blocking and hold off sending
> more data to the device until you're sure there's room in its buffers. I
> guess a workqueue should do fine.
We're currently testing an updated driver based on a workqueue
(work_struct) and it's working fine.
The issue that Corentin reported is unrelated to the workqueue part.
At this point we're thinking it may be a regression in linux-next, but
we're running more tests to verify this.


Best regards,
Martin

Re: [PATCH 1/1 v7] usb: serial: add support for CH348

[Johan Hovold]

On Tue, Jan 14, 2025 at 10:40:56PM +0100, Martin Blumenstingl wrote:

> > Not sure what Corentin has been preparing, but yeah, you need some kind
> > of deferred mechanism to make write() non-blocking and hold off sending
> > more data to the device until you're sure there's room in its buffers. I
> > guess a workqueue should do fine.

> We're currently testing an updated driver based on a workqueue
> (work_struct) and it's working fine.
> The issue that Corentin reported is unrelated to the workqueue part.
> At this point we're thinking it may be a regression in linux-next, but
> we're running more tests to verify this.

You don't need to test the driver against linux-next, rc7 should do just
fine for a driver like this.

Let's see what the new implementation looks like. Perhaps you don't even
need to use a workqueue (e.g. send off the single URB from the
completion callback when you get a notification that the transmitter has
emptied).

Johan