USB: serial: ch341: Add support for limited chips (was: Add support for HL340 devices)

PATCH In review Michael Hanselmann 2 participants 23 messages Mar 31, 2020 – Jul 6, 2020

Subsystems (2)

USB USB serial

lore

Share & tools

lore

Michael Hanselmann

author

2020-03-31 23:37 UTC #0 lore raw

v2 cover USB: serial: ch341: Add support for limited chips (was: Add support for HL340 devices)

A subset of CH341 devices does not support all features, namely the prescaler is limited to a reduced precision and there is no support for sending a RS232 break condition.

These devices can usually be identified by an imprint of "340" on the turquoise-colored plug. They're also sometimes called "HL340", but not all such devices are affected.

This series takes multiple steps to support the limited chips:

* Remove special cases from clock prescaler computation
* Detect limited devices by trying to read a register related to sending a break condition
* Amend clock prescaler computation to only use working values on limited chips
* Simulate an RS232 break condition by temporarily lowering the baud rate and sending a NUL byte

Michael Hanselmann (6):
USB: serial: ch341: Reduce special cases in clock calculation
USB: serial: ch341: Add basis for quirk detection
USB: serial: ch341: Limit prescaler on quirky chips
USB: serial: ch341: Name prescaler, divisor registers
USB: serial: ch341: Compute minimum baud rate
USB: serial: ch341: Simulate break condition if not supported

 drivers/usb/serial/ch341.c | 294 ++++++++++++++++++++++++++++++-------
 1 file changed, 244 insertions(+), 50 deletions(-)

Signature · 1 line

2.20.1

Michael Hanselmann

author

2020-03-31 23:37 UTC #1 lore raw

PATCH v2 1/6 USB: serial: ch341: Reduce special cases in clock calculation

The CH341 clock prescaler and divisor calculation contained two special cases, one to halve the clock rate if the divisor was outside the allowed range, and the other to prefer an odd divisor by halving too (only one special case would be applied at once).

A utility function is introduced to keep the logic for calculating the divisor and testing whether it's within the permitted range out of "ch341_get_divisor". The latter takes care the first special case. Calling the utility function twice in a loop allows preferring odd divisors.

There's another motivation for making this change: a subset of all CH341 devices doesn't work correctly with a few prescaler values. Filling the minimum rate lookup table built at compile-time with all possible values allows a later patch to call the utility function and absolves it from having to consider the aforementioned special cases.

Tested by comparing the results of the previous code and the changed code for every baud rate value from the minimum of 46 to the maximum of 3000000 bps. The computed divisors remain the same.

Signed-off-by Michael Hanselmann <[email protected]>

---
 drivers/usb/serial/ch341.c | 90 ++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 34 deletions(-)

drivers/usb/serial/ch341.c +56 ~~-34~~

index c5ecdcd51ffc..85e7125d0194 100644
@@ -140,15 +140,50 @@ static int ch341_control_in(struct usb_device *dev,
 
 #define CH341_CLKRATE		48000000
 #define CH341_CLK_DIV(ps, fact)	(1 << (12 - 3 * (ps) - (fact)))
-#define CH341_MIN_RATE(ps)	(CH341_CLKRATE / (CH341_CLK_DIV((ps), 1) * 512))
+#define CH341_MIN_RATE(ps, fact) \
+	(CH341_CLKRATE / (CH341_CLK_DIV((ps), (fact)) * 256))
 
+/* Minimum baud rate for given prescaler values */
 static const speed_t ch341_min_rates[] = {
-	CH341_MIN_RATE(0),
-	CH341_MIN_RATE(1),
-	CH341_MIN_RATE(2),
-	CH341_MIN_RATE(3),
+	CH341_MIN_RATE(0, 0),
+	CH341_MIN_RATE(0, 1),
+	CH341_MIN_RATE(1, 0),
+	CH341_MIN_RATE(1, 1),
+	CH341_MIN_RATE(2, 0),
+	CH341_MIN_RATE(2, 1),
+	CH341_MIN_RATE(3, 0),
+	CH341_MIN_RATE(3, 1),
 };
 
+static int ch341_calc_divisor(speed_t speed, unsigned int ps,
+		unsigned int fact, unsigned int *div, unsigned int *clk_div)
+{
+	const unsigned int offset = ps * 2 + fact;
+
+	if (offset >= ARRAY_SIZE(ch341_min_rates)) {
+		return -EINVAL;
+	}
+
+	if (speed > ch341_min_rates[offset]) {
+		const unsigned int min_div = fact == 0 ? 2 : 9;
+
+		/* Determine corresponding divisor, rounding down. */
+		*clk_div = CH341_CLK_DIV(ps, fact);
+		*div = CH341_CLKRATE / (*clk_div * speed);
+
+		/*
+		 * Determine whether divisor is in supported range. The upper
+		 * limit is kept one below the maximum to enable picking the
+		 * next rate if it's more accurate.
+		 */
+		if (*div >= min_div && *div < 256) {
+			return 0;
+		}
+	}
+
+	return -ERANGE;
+}
+
 /*
  * The device line speed is given by the following equation:
  *

@@ -171,30 +206,27 @@ static int ch341_get_divisor(speed_t speed)
 	speed = clamp(speed, 46U, 3000000U);
 
 	/*
-	 * Start with highest possible base clock (fact = 1) that will give a
-	 * divisor strictly less than 512.
+	 * Start with highest possible base clock and find a divisor for the
+	 * requested baud rate.
 	 */
-	fact = 1;
-	for (ps = 3; ps >= 0; ps--) {
-		if (speed > ch341_min_rates[ps])
+	for (ps = 3; ps >= 0; --ps) {
+		if (ch341_calc_divisor(speed, ps, 1U, &div, &clk_div) == 0) {
+			fact = 1;
 			break;
-	}
-
-	if (ps < 0)
-		return -EINVAL;
-
-	/* Determine corresponding divisor, rounding down. */
-	clk_div = CH341_CLK_DIV(ps, fact);
-	div = CH341_CLKRATE / (clk_div * speed);
+		}
 
-	/* Halve base clock (fact = 0) if required. */
-	if (div < 9 || div > 255) {
-		div /= 2;
-		clk_div *= 2;
-		fact = 0;
+		/*
+		 * Prefer half base clock (fact = 0) before trying lower
+		 * prescaler values. This makes the receiver more tolerant to
+		 * errors.
+		 */
+		if (ch341_calc_divisor(speed, ps, 0U, &div, &clk_div) == 0) {
+			fact = 0;
+			break;
+		}
 	}
 
-	if (div < 2)
+	if (ps < 0 || div < 2 || div > 255)
 		return -EINVAL;
 
 	/*

@@ -205,16 +237,6 @@ static int ch341_get_divisor(speed_t speed)
 			16 * speed - 16 * CH341_CLKRATE / (clk_div * (div + 1)))
 		div++;
 
-	/*
-	 * Prefer lower base clock (fact = 0) if even divisor.
-	 *
-	 * Note that this makes the receiver more tolerant to errors.
-	 */
-	if (fact == 1 && div % 2 == 0) {
-		div /= 2;
-		fact = 0;
-	}
-
 	return (0x100 - div) << 8 | fact << 2 | ps;
 }

Signature · 1 line

2.20.1

Michael Hanselmann

author

2020-03-31 23:37 UTC #2 lore raw

PATCH v2 2/6 USB: serial: ch341: Add basis for quirk detection

In reply to Michael Hanselmann

A subset of CH341 devices does not support all features, namely the prescaler is limited to a reduced precision and there is no support for sending a RS232 break condition. This patch adds a detection function which will be extended to return quirk flags as they're implemented.

The author's affected device has an imprint of "340" on the turquoise-colored plug, but not all such devices appear to be affected.

Signed-off-by Michael Hanselmann <[email protected]>

---
 drivers/usb/serial/ch341.c | 49 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

drivers/usb/serial/ch341.c +49 -0

index 85e7125d0194..9c839f67c3d4 100644
@@ -87,6 +87,7 @@ struct ch341_private {
 	u8 mcr;
 	u8 msr;
 	u8 lcr;
+	u8 quirks;
 };
 
 static void ch341_set_termios(struct tty_struct *tty,

@@ -330,6 +331,45 @@ out:	kfree(buffer);
 	return r;
 }
 
+static int ch341_detect_quirks(struct usb_device *dev)
+{
+	const unsigned int size = 2;
+	char *buffer;
+	int r;
+
+	buffer = kmalloc(size, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	/*
+	 * A subset of CH34x devices does not support all features. The
+	 * prescaler is limited and there is no support for sending a RS232
+	 * break condition. A read failure when trying to set up the latter is
+	 * used to detect these devices.
+	 */
+	r = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), CH341_REQ_READ_REG,
+			    USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+			    CH341_REG_BREAK, 0, buffer, size, DEFAULT_TIMEOUT);
+	if (r == -EPIPE) {
+		dev_dbg(&dev->dev, "%s - reading break condition register"
+			" failed (%d)\n", __func__, r);
+		r = 0;
+		goto out;
+	}
+
+	if (r < 0) {
+		dev_err(&dev->dev, "%s - USB control read error (%d)\n",
+			__func__, r);
+		goto out;
+	}
+
+	r = 0;
+
+out:
+	kfree(buffer);
+	return r;
+}
+
 static int ch341_port_probe(struct usb_serial_port *port)
 {
 	struct ch341_private *priv;

@@ -351,6 +391,15 @@ static int ch341_port_probe(struct usb_serial_port *port)
 	if (r < 0)
 		goto error;
 
+	r = ch341_detect_quirks(port->serial->dev);
+	if (r < 0)
+		goto error;
+	if (r != 0) {
+		dev_dbg(&port->serial->dev->dev,
+			"%s - enabling quirks flags %08x\n", __func__, r);
+		priv->quirks |= r;
+	}
+
 	usb_set_serial_port_data(port, priv);
 	return 0;

Signature · 1 line

2.20.1

Johan Hovold

maintainer

2020-05-14 14:09 UTC #3 lore raw

Re: [PATCH v2 2/6] USB: serial: ch341: Add basis for quirk detection

Quoted 9 lines · Michael Hanselmann

A subset of CH341 devices does not support all features, namely the prescaler is limited to a reduced precision and there is no support for sending a RS232 break condition. This patch adds a detection function which will be extended to return quirk flags as they're implemented.
The author's affected device has an imprint of "340" on the turquoise-colored plug, but not all such devices appear to be affected.
Signed-off-by Michael Hanselmann <[email protected]>

I've applied this one now after making some minor changes.

drivers/usb/serial/ch341.c struct ch341_private { · line 90

Quoted 13 lines

---
 drivers/usb/serial/ch341.c | 49 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

drivers/usb/serial/ch341.c +1 -0

index 85e7125d0194..9c839f67c3d4 100644
@@ -87,6 +87,7 @@ struct ch341_private {
 	u8 mcr;
 	u8 msr;
 	u8 lcr;
+	u8 quirks;

I used unsigned long to avoid any false sharing with the locked fields even if that's likely not an issue here.

Quoted 19 lines

drivers/usb/serial/ch341.c +12 -0

 };
 
 static void ch341_set_termios(struct tty_struct *tty,

@@ -330,6 +331,45 @@ out:	kfree(buffer);
 	return r;
 }
 
+static int ch341_detect_quirks(struct usb_device *dev)
+{
+	const unsigned int size = 2;
+	char *buffer;
+	int r;
+
+	buffer = kmalloc(size, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	/*
+	 * A subset of CH34x devices does not support all features. The

drivers/usb/serial/ch341.c out: kfree(buffer); · line 355

Quoted 10 lines

drivers/usb/serial/ch341.c +10 -0

+	 * prescaler is limited and there is no support for sending a RS232
+	 * break condition. A read failure when trying to set up the latter is
+	 * used to detect these devices.
+	 */
+	r = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), CH341_REQ_READ_REG,
+			    USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+			    CH341_REG_BREAK, 0, buffer, size, DEFAULT_TIMEOUT);
+	if (r == -EPIPE) {
+		dev_dbg(&dev->dev, "%s - reading break condition register"
+			" failed (%d)\n", __func__, r);

Log message strings should not be broken (even if that violates the 80 column rule) so that they are easier to grep for.

No need need to include the errno, here and reworded the message.

I also used passed in the port structure and used the port->dev for logging (I know, the driver isn't consistent here).

drivers/usb/serial/ch341.c out: kfree(buffer); · line 360

Quoted 5 lines

drivers/usb/serial/ch341.c +5 -0
+		r = 0;
+		goto out;
+	}
+
+	if (r < 0) {

We generally need to check for short transfers as well when reading.

Quoted 16 lines

drivers/usb/serial/ch341.c +12 -0

+		dev_err(&dev->dev, "%s - USB control read error (%d)\n",
+			__func__, r);
+		goto out;
+	}
+
+	r = 0;
+
+out:
+	kfree(buffer);
+	return r;
+}
+
 static int ch341_port_probe(struct usb_serial_port *port)
 {
 	struct ch341_private *priv;

drivers/usb/serial/ch341.c static int ch341_port_probe(struct usb_serial_port *port) · line 400

Quoted 10 lines

drivers/usb/serial/ch341.c +7 -0

@@ -351,6 +391,15 @@ static int ch341_port_probe(struct usb_serial_port *port)
 	if (r < 0)
 		goto error;
 
+	r = ch341_detect_quirks(port->serial->dev);
+	if (r < 0)
+		goto error;
+	if (r != 0) {
+		dev_dbg(&port->serial->dev->dev,
+			"%s - enabling quirks flags %08x\n", __func__, r);
+		priv->quirks |= r;

And finally I moved this bit into the helper itself.

drivers/usb/serial/ch341.c static int ch341_port_probe(struct usb_serial_port *port) · line 404

Quoted 4 lines

drivers/usb/serial/ch341.c +2 -0

+	}
+
 	usb_set_serial_port_data(port, priv);
 	return 0;

Johan

Michael Hanselmann

author

2020-03-31 23:37 UTC #4 lore raw

PATCH v2 3/6 USB: serial: ch341: Limit prescaler on quirky chips

In reply to Michael Hanselmann

A subset of all CH341 devices stop responding to bulk transfers, usually after the third byte, when the highest prescaler bit (0b100) is set. There is one exception, namely a prescaler of exactly 0b111 (fact=1, ps=3). Limit these to the lower prescaler values at the cost of timing precision.

Rates above 46875 baud use the same timings as the normal algorithm. Below that the worst difference between desired and actual baud rate is 2.17 percentage points. The worst difference is 1.06 p.p. when only looking at divisors differing from the normal algorithm.

Signed-off-by Michael Hanselmann <[email protected]>

---
 drivers/usb/serial/ch341.c | 70 ++++++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 18 deletions(-)

drivers/usb/serial/ch341.c +52 ~~-18~~

index 9c839f67c3d4..67a5d4c3df42 100644
@@ -24,6 +24,8 @@
 #define DEFAULT_BAUD_RATE 9600
 #define DEFAULT_TIMEOUT   1000
 
+#define CH341_QUIRK_LIMITED_PRESCALER 0x01
+
 /* flags for IO-Bits */
 #define CH341_BIT_RTS (1 << 6)
 #define CH341_BIT_DTR (1 << 5)

@@ -195,35 +197,67 @@ static int ch341_calc_divisor(speed_t speed, unsigned int ps,
  *		2 <= div <= 256 if fact = 0, or
  *		9 <= div <= 256 if fact = 1
  */
-static int ch341_get_divisor(speed_t speed)
+static int ch341_get_divisor(struct ch341_private *priv)
 {
+	speed_t speed;
 	unsigned int fact, div, clk_div;
 	int ps;
 
+	speed = priv->baud_rate;
+
 	/*
 	 * Clamp to supported range, this makes the (ps < 0) and (div < 2)
 	 * sanity checks below redundant.
 	 */
 	speed = clamp(speed, 46U, 3000000U);
 
-	/*
-	 * Start with highest possible base clock and find a divisor for the
-	 * requested baud rate.
-	 */
-	for (ps = 3; ps >= 0; --ps) {
-		if (ch341_calc_divisor(speed, ps, 1U, &div, &clk_div) == 0) {
-			fact = 1;
-			break;
-		}
-
+	if (priv->quirks & CH341_QUIRK_LIMITED_PRESCALER) {
 		/*
-		 * Prefer half base clock (fact = 0) before trying lower
-		 * prescaler values. This makes the receiver more tolerant to
-		 * errors.
+		 * A subset of all CH34x devices stop responding to bulk
+		 * transfers when configured with certain prescaler values.
+		 *
+		 * fact=0, ps=0..3: Works
+		 * fact=1, ps=0..2: Unreliable
+		 * fact=1, ps=3: Works
+		 *
+		 * Limit these devices to working prescaler values at the cost
+		 * of precision for speeds up to 46875 baud above which
+		 * fact = 1 with ps = 3 is used.
 		 */
-		if (ch341_calc_divisor(speed, ps, 0U, &div, &clk_div) == 0) {
+		if (ch341_calc_divisor(speed, 3U, 1U, &div, &clk_div) == 0) {
+			ps = 3U;
+			fact = 1U;
+		} else {
 			fact = 0;
-			break;
+
+			for (ps = 3; ps >= 0; --ps) {
+				if (ch341_calc_divisor(speed, ps, fact,
+						       &div, &clk_div) == 0)
+					break;
+			}
+		}
+	} else {
+		/*
+		 * Start with highest possible base clock and find a divisor
+		 * for the requested baud rate.
+		 */
+		for (ps = 3; ps >= 0; --ps) {
+			if (ch341_calc_divisor(speed, ps, 1U,
+					       &div, &clk_div) == 0) {
+				fact = 1;
+				break;
+			}
+
+			/*
+			 * Prefer half base clock (fact = 0) before trying
+			 * lower prescaler values. This makes the receiver more
+			 * tolerant to errors.
+			 */
+			if (ch341_calc_divisor(speed, ps, 0U,
+					       &div, &clk_div) == 0) {
+				fact = 0;
+				break;
+			}
 		}
 	}

@@ -250,7 +284,7 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 	if (!priv->baud_rate)
 		return -EINVAL;
 
-	val = ch341_get_divisor(priv->baud_rate);
+	val = ch341_get_divisor(priv);
 	if (val < 0)
 		return -EINVAL;

@@ -353,7 +387,7 @@ static int ch341_detect_quirks(struct usb_device *dev)
 	if (r == -EPIPE) {
 		dev_dbg(&dev->dev, "%s - reading break condition register"
 			" failed (%d)\n", __func__, r);
-		r = 0;
+		r = CH341_QUIRK_LIMITED_PRESCALER;
 		goto out;
 	}

Signature · 1 line

2.20.1

Johan Hovold

maintainer

2020-05-14 14:17 UTC #5 lore raw

Re: [PATCH v2 3/6] USB: serial: ch341: Limit prescaler on quirky chips

Quoted 123 lines · Michael Hanselmann

Signed-off-by Michael Hanselmann <[email protected]>

---
 drivers/usb/serial/ch341.c | 70 ++++++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 18 deletions(-)

drivers/usb/serial/ch341.c +51 ~~-17~~

index 9c839f67c3d4..67a5d4c3df42 100644
@@ -24,6 +24,8 @@
 #define DEFAULT_BAUD_RATE 9600
 #define DEFAULT_TIMEOUT   1000
 
+#define CH341_QUIRK_LIMITED_PRESCALER 0x01
+
 /* flags for IO-Bits */
 #define CH341_BIT_RTS (1 << 6)
 #define CH341_BIT_DTR (1 << 5)

@@ -195,35 +197,67 @@ static int ch341_calc_divisor(speed_t speed, unsigned int ps,
  *		2 <= div <= 256 if fact = 0, or
  *		9 <= div <= 256 if fact = 1
  */
-static int ch341_get_divisor(speed_t speed)
+static int ch341_get_divisor(struct ch341_private *priv)
 {
+	speed_t speed;
 	unsigned int fact, div, clk_div;
 	int ps;
 
+	speed = priv->baud_rate;
+
 	/*
 	 * Clamp to supported range, this makes the (ps < 0) and (div < 2)
 	 * sanity checks below redundant.
 	 */
 	speed = clamp(speed, 46U, 3000000U);
 
-	/*
-	 * Start with highest possible base clock and find a divisor for the
-	 * requested baud rate.
-	 */
-	for (ps = 3; ps >= 0; --ps) {
-		if (ch341_calc_divisor(speed, ps, 1U, &div, &clk_div) == 0) {
-			fact = 1;
-			break;
-		}
-
+	if (priv->quirks & CH341_QUIRK_LIMITED_PRESCALER) {
 		/*
-		 * Prefer half base clock (fact = 0) before trying lower
-		 * prescaler values. This makes the receiver more tolerant to
-		 * errors.
+		 * A subset of all CH34x devices stop responding to bulk
+		 * transfers when configured with certain prescaler values.
+		 *
+		 * fact=0, ps=0..3: Works
+		 * fact=1, ps=0..2: Unreliable
+		 * fact=1, ps=3: Works
+		 *
+		 * Limit these devices to working prescaler values at the cost
+		 * of precision for speeds up to 46875 baud above which
+		 * fact = 1 with ps = 3 is used.
 		 */
-		if (ch341_calc_divisor(speed, ps, 0U, &div, &clk_div) == 0) {
+		if (ch341_calc_divisor(speed, 3U, 1U, &div, &clk_div) == 0) {
+			ps = 3U;
+			fact = 1U;
+		} else {
 			fact = 0;
-			break;
+
+			for (ps = 3; ps >= 0; --ps) {
+				if (ch341_calc_divisor(speed, ps, fact,
+						       &div, &clk_div) == 0)
+					break;
+			}
+		}
+	} else {
+		/*
+		 * Start with highest possible base clock and find a divisor
+		 * for the requested baud rate.
+		 */
+		for (ps = 3; ps >= 0; --ps) {
+			if (ch341_calc_divisor(speed, ps, 1U,
+					       &div, &clk_div) == 0) {
+				fact = 1;
+				break;
+			}
+
+			/*
+			 * Prefer half base clock (fact = 0) before trying
+			 * lower prescaler values. This makes the receiver more
+			 * tolerant to errors.
+			 */
+			if (ch341_calc_divisor(speed, ps, 0U,
+					       &div, &clk_div) == 0) {
+				fact = 0;
+				break;
+			}
 		}
 	}

@@ -250,7 +284,7 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 	if (!priv->baud_rate)
 		return -EINVAL;
 
-	val = ch341_get_divisor(priv->baud_rate);
+	val = ch341_get_divisor(priv);
 	if (val < 0)
 		return -EINVAL;

drivers/usb/serial/ch341.c static int ch341_detect_quirks(struct usb_device *dev) · line 392

Quoted 7 lines

drivers/usb/serial/ch341.c +1 -1

@@ -353,7 +387,7 @@ static int ch341_detect_quirks(struct usb_device *dev)
 	if (r == -EPIPE) {
 		dev_dbg(&dev->dev, "%s - reading break condition register"
 			" failed (%d)\n", __func__, r);
-		r = 0;
+		r = CH341_QUIRK_LIMITED_PRESCALER;
 		goto out;
 	}

This can be implemented in a more compact way which makes the algorithm easier to follow and doesn't depend on your patch 1/6 either. With a smaller fix we can get this backported to stable as well.

I've verified that the below works here on top of your (slightly modified) 2/6. Would you mind giving it a try on your end as well?

Johan

From ea23730616b6406101d4efeb12e1ae2312dd20c9 Mon Sep 17 00:00:00 2001
From: Johan Hovold <[email protected]>
Date: Thu, 14 May 2020 11:36:45 +0200
Subject: [PATCH] USB: serial: ch341: fix lockup of devices with limited
prescaler

Michael Hanselmann reports that

[a] subset of all CH341 devices stop responding to bulk transfers, usually after the third byte, when the highest prescaler bit (0b100) is set. There is one exception, namely a prescaler of exactly 0b111 (fact=1, ps=3).

Fix this by forcing a lower base clock (fact = 0) whenever needed.

This specifically makes the standard rates 110, 134 and 200 bps work again with these devices.

Fixes 35714565089e ("USB: serial: ch341: reimplement line-speed handling")

Cc stable <[email protected]> # 5.5

Reported-by Michael Hanselmann <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/serial/ch341.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

drivers/usb/serial/ch341.c +12 -3

index f2b93f4554a7..89675ee29645 100644
@@ -73,6 +73,8 @@
 #define CH341_LCR_CS6          0x01
 #define CH341_LCR_CS5          0x00
 
+#define CH341_QUIRK_LIMITED_PRESCALER	BIT(0)
+
 static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(0x4348, 0x5523) },
 	{ USB_DEVICE(0x1a86, 0x7523) },

@@ -160,9 +162,11 @@ static const speed_t ch341_min_rates[] = {
  *		2 <= div <= 256 if fact = 0, or
  *		9 <= div <= 256 if fact = 1
  */
-static int ch341_get_divisor(speed_t speed)
+static int ch341_get_divisor(struct ch341_private *priv)
 {
 	unsigned int fact, div, clk_div;
+	speed_t speed = priv->baud_rate;
+	bool force_fact0 = false;
 	int ps;
 
 	/*

@@ -188,8 +192,12 @@ static int ch341_get_divisor(speed_t speed)
 	clk_div = CH341_CLK_DIV(ps, fact);
 	div = CH341_CLKRATE / (clk_div * speed);
 
+	/* Some devices require a lower base clock if ps < 3. */
+	if (ps < 3 && (priv->quirks & CH341_QUIRK_LIMITED_PRESCALER))
+		force_fact0 = true;
+
 	/* Halve base clock (fact = 0) if required. */
-	if (div < 9 || div > 255) {
+	if (div < 9 || div > 255 || force_fact0) {
 		div /= 2;
 		clk_div *= 2;
 		fact = 0;

@@ -228,7 +236,7 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 	if (!priv->baud_rate)
 		return -EINVAL;
 
-	val = ch341_get_divisor(priv->baud_rate);
+	val = ch341_get_divisor(priv);
 	if (val < 0)
 		return -EINVAL;

@@ -333,6 +341,7 @@ static int ch341_detect_quirks(struct usb_serial_port *port)
 			    CH341_REG_BREAK, 0, buffer, size, DEFAULT_TIMEOUT);
 	if (r == -EPIPE) {
 		dev_dbg(&port->dev, "break control not supported\n");
+		quirks = CH341_QUIRK_LIMITED_PRESCALER;
 		r = 0;
 		goto out;
 	}

Signature · 1 line

2.26.2

Johan Hovold

maintainer

2020-05-27 13:17 UTC #6 lore raw

Re: [PATCH v2 3/6] USB: serial: ch341: Limit prescaler on quirky chips

Quoted 78 lines · Johan Hovold

On Tue, Mar 31, 2020 at 11:37:19PM +0000, Michael Hanselmann wrote:
From ea23730616b6406101d4efeb12e1ae2312dd20c9 Mon Sep 17 00:00:00 2001
From: Johan Hovold <[email protected]>
Date: Thu, 14 May 2020 11:36:45 +0200
Subject: [PATCH] USB: serial: ch341: fix lockup of devices with limited
prescaler
Michael Hanselmann reports that
[a] subset of all CH341 devices stop responding to bulk transfers, usually after the third byte, when the highest prescaler bit (0b100) is set. There is one exception, namely a prescaler of exactly 0b111 (fact=1, ps=3).
Fix this by forcing a lower base clock (fact = 0) whenever needed.
This specifically makes the standard rates 110, 134 and 200 bps work again with these devices.
Fixes 35714565089e ("USB: serial: ch341: reimplement line-speed handling")
Cc stable <[email protected]> # 5.5
Reported-by Michael Hanselmann <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/ch341.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
drivers/usb/serial/ch341.c +11 -3
index f2b93f4554a7..89675ee29645 100644
@@ -73,6 +73,8 @@
 #define CH341_LCR_CS6          0x01
 #define CH341_LCR_CS5          0x00
 
+#define CH341_QUIRK_LIMITED_PRESCALER	BIT(0)
+
 static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(0x4348, 0x5523) },
 	{ USB_DEVICE(0x1a86, 0x7523) },
@@ -160,9 +162,11 @@ static const speed_t ch341_min_rates[] = {
  *		2 <= div <= 256 if fact = 0, or
  *		9 <= div <= 256 if fact = 1
  */
-static int ch341_get_divisor(speed_t speed)
+static int ch341_get_divisor(struct ch341_private *priv)
 {
 	unsigned int fact, div, clk_div;
+	speed_t speed = priv->baud_rate;
+	bool force_fact0 = false;
 	int ps;
 
 	/*
@@ -188,8 +192,12 @@ static int ch341_get_divisor(speed_t speed)
 	clk_div = CH341_CLK_DIV(ps, fact);
 	div = CH341_CLKRATE / (clk_div * speed);
 
+	/* Some devices require a lower base clock if ps < 3. */
+	if (ps < 3 && (priv->quirks & CH341_QUIRK_LIMITED_PRESCALER))
+		force_fact0 = true;
+
 	/* Halve base clock (fact = 0) if required. */
-	if (div < 9 || div > 255) {
+	if (div < 9 || div > 255 || force_fact0) {
 		div /= 2;
 		clk_div *= 2;
 		fact = 0;
@@ -228,7 +236,7 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 	if (!priv->baud_rate)
 		return -EINVAL;
 
-	val = ch341_get_divisor(priv->baud_rate);
+	val = ch341_get_divisor(priv);
 	if (val < 0)
 		return -EINVAL;
 

drivers/usb/serial/ch341.c static int ch341_detect_quirks(struct usb_serial_port *port) · line 347

Quoted 7 lines

drivers/usb/serial/ch341.c +1 -0

@@ -333,6 +341,7 @@ static int ch341_detect_quirks(struct usb_serial_port *port)
 			    CH341_REG_BREAK, 0, buffer, size, DEFAULT_TIMEOUT);
 	if (r == -EPIPE) {
 		dev_dbg(&port->dev, "break control not supported\n");
+		quirks = CH341_QUIRK_LIMITED_PRESCALER;
 		r = 0;
 		goto out;
 	}

I've applied the above fix on top of your quirk-detection patch so that we can get this into 5.8 and backported to stable.

Johan

Michael Hanselmann

author

2020-05-27 15:42 UTC #7 lore raw

Re: [PATCH v2 3/6] USB: serial: ch341: Limit prescaler on quirky chips

Quoted 2 lines · Johan Hovold

I've applied the above fix on top of your quirk-detection patch so that we can get this into 5.8 and backported to stable.

I tested ch341 on usb-next at c432df15591958. The adapter kept working for all tested baud rates from 75 to 200000 in 75bd intervals.

Will reply to your comments on the other patches in this series soon. Didn't get around to it yet.

Michael

Johan Hovold

maintainer

2020-05-29 07:16 UTC #8 lore raw

Re: [PATCH v2 3/6] USB: serial: ch341: Limit prescaler on quirky chips

Quoted 6 lines · Michael Hanselmann

Quoted 2 lines · Johan Hovold
I've applied the above fix on top of your quirk-detection patch so that we can get this into 5.8 and backported to stable.
I tested ch341 on usb-next at c432df15591958. The adapter kept working for all tested baud rates from 75 to 200000 in 75bd intervals.

Great, thanks for testing!

Johan

Michael Hanselmann

author

2020-03-31 23:37 UTC #9 lore raw

PATCH v2 4/6 USB: serial: ch341: Name prescaler, divisor registers

In reply to Michael Hanselmann

Add constants for the prescaler and divisor registers.

The 0x25 register is only used by CH341 chips before version 0x30 and is involved in configuring the line control parameters. It's not known to the author whether there any such chips in the wild, and the driver never supported them (other registers are also treated differently). The alternative would've been to not set the register, but that may have unintended effects.

Signed-off-by Michael Hanselmann <[email protected]>

---
 drivers/usb/serial/ch341.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

drivers/usb/serial/ch341.c +14 -2

index 67a5d4c3df42..9407e12d9fbc 100644
@@ -61,7 +61,11 @@
 #define CH341_REQ_MODEM_CTRL   0xA4
 
 #define CH341_REG_BREAK        0x05
+#define CH341_REG_PRESCALER    0x12
+#define CH341_REG_DIVISOR      0x13
 #define CH341_REG_LCR          0x18
+#define CH341_REG_LCR2         0x25
+
 #define CH341_NBREAK_BITS      0x01
 
 #define CH341_LCR_ENABLE_RX    0x80

@@ -294,11 +298,19 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 	 */
 	val |= BIT(7);
 
-	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x1312, val);
+	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
+			      CH341_REG_DIVISOR << 8 | CH341_REG_PRESCALER,
+			      val);
 	if (r)
 		return r;
 
-	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x2518, lcr);
+	/*
+	 * Chip versions before version 0x30 (read using
+	 * CH341_REQ_READ_VERSION) used separate registers for line control.
+	 * 0x30 and above use CH341_REG_LCR only.
+	 */
+	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
+			      CH341_REG_LCR2 << 8 | CH341_REG_LCR, lcr);
 	if (r)
 		return r;

Signature · 1 line

2.20.1

Johan Hovold

maintainer

2020-05-14 14:24 UTC #10 lore raw

Re: [PATCH v2 4/6] USB: serial: ch341: Name prescaler, divisor registers

Quoted 1 line · Michael Hanselmann

Add constants for the prescaler and divisor registers.

...and document and name register 0x25, and put the LCR define to more use. I still thinks this should go in its own patch as your not replacing all magic register constants (or at least be mentioned in the commit message).

Quoted 6 lines

The 0x25 register is only used by CH341 chips before version 0x30 and is involved in configuring the line control parameters. It's not known to the author whether there any such chips in the wild, and the driver never supported them (other registers are also treated differently). The alternative would've been to not set the register, but that may have unintended effects.

How did you come to those conclusions? I see this register being written the value zero in some older version of the vendor driver, but not in more recent ones.

Are you sure it's at all related to LCR?

Quoted 30 lines

Signed-off-by Michael Hanselmann <[email protected]>

---
 drivers/usb/serial/ch341.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

drivers/usb/serial/ch341.c +7 -1

index 67a5d4c3df42..9407e12d9fbc 100644
@@ -61,7 +61,11 @@
 #define CH341_REQ_MODEM_CTRL   0xA4
 
 #define CH341_REG_BREAK        0x05
+#define CH341_REG_PRESCALER    0x12
+#define CH341_REG_DIVISOR      0x13
 #define CH341_REG_LCR          0x18
+#define CH341_REG_LCR2         0x25
+
 #define CH341_NBREAK_BITS      0x01
 
 #define CH341_LCR_ENABLE_RX    0x80

@@ -294,11 +298,19 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 	 */
 	val |= BIT(7);
 
-	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x1312, val);
+	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
+			      CH341_REG_DIVISOR << 8 | CH341_REG_PRESCALER,
+			      val);
 	if (r)

drivers/usb/serial/ch341.c static int ch341_set_baudrate_lcr(struct usb_device *dev, · line 313

Quoted 10 lines

drivers/usb/serial/ch341.c +7 -1

 		return r;
 
-	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x2518, lcr);
+	/*
+	 * Chip versions before version 0x30 (read using
+	 * CH341_REQ_READ_VERSION) used separate registers for line control.
+	 * 0x30 and above use CH341_REG_LCR only.
+	 */
+	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
+			      CH341_REG_LCR2 << 8 | CH341_REG_LCR, lcr);

Keeping "0x25" here to indicate that it's purpose is still not known should be ok too.

drivers/usb/serial/ch341.c static int ch341_set_baudrate_lcr(struct usb_device *dev, · line 315

Quoted 2 lines

drivers/usb/serial/ch341.c +0 -0
 	if (r)
 		return r;

Johan

Michael Hanselmann

author

2020-05-27 20:59 UTC #11 lore raw

Re: [PATCH v2 4/6] USB: serial: ch341: Name prescaler, divisor registers

Quoted 7 lines · Johan Hovold

Quoted 1 line · Michael Hanselmann
Add constants for the prescaler and divisor registers.
...and document and name register 0x25, and put the LCR define to more use. I still thinks this should go in its own patch as your not replacing all magic register constants (or at least be mentioned in the commit message).

Updated the commit message with more details, see below too. Thanks for the suggestion and review!

Quoted 7 lines

Quoted 6 lines
The 0x25 register is only used by CH341 chips before version 0x30 and is involved in configuring the line control parameters. It's not known to the author whether there any such chips in the wild, and the driver never supported them (other registers are also treated differently). The alternative would've been to not set the register, but that may have unintended effects.

Quoted 5 lines

How did you come to those conclusions? I see this register being written the value zero in some older version of the vendor driver, but not in more recent ones.
Are you sure it's at all related to LCR?

I am looking at version 3.50.2014.8 from 2019-03-04 of CH341S64.SYS, the vendor driver. The function configuring registers 0x18 and 0x25 starts at offset 0x119f0.

For chip versions 0x30 and above 0x25 is always set to 0. For versions before 0x30 the values stored in register 0x25 depend on stop bit, partity and word length settings.

Quoted 11 lines

Review

Quoted 8 lines

(quoted diff) +7 -1

-	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x2518, lcr);
+	/*
+	 * Chip versions before version 0x30 (read using
+	 * CH341_REQ_READ_VERSION) used separate registers for line control.
+	 * 0x30 and above use CH341_REG_LCR only.
+	 */
+	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
+			      CH341_REG_LCR2 << 8 | CH341_REG_LCR, lcr);

Keeping "0x25" here to indicate that it's purpose is still not known should be ok too.

Considering that the purpose is known (now) I've updated the comment. I'm not going to attempt to implement the appropriate configuration for the chip versions before 0x30.

Michael

---

From aa597afe1e9cf5641b94b377ce63248c2d0d677a Mon Sep 17 00:00:00 2001
From: Michael Hanselmann <[email protected]>
Date: Wed, 27 May 2020 22:56:54 +0200
Subject: [PATCH] USB: serial: ch341: Name prescaler, divisor registers

Add constants for the prescaler and divisor registers. Document and name register 0x25, and put the LCR define to more use.

The 0x25 register (CH341_REG_LCR2) is only used by CH341 chips before version 0x30 and is involved in configuring the line control parameters. It's not known to the author whether there any such chips in the wild, and Linux' ch341 driver never supported them. For chip version 0x30 and above the 0x25 register is always set to zero. The alternative would've been to not set the register at all, but that may have unintended effects.

Signed-off-by Michael Hanselmann <[email protected]>

---
 drivers/usb/serial/ch341.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

drivers/usb/serial/ch341.c +15 -2

index 89675ee29645..97214e1ec5d2 100644
@@ -59,7 +59,11 @@
 #define CH341_REQ_MODEM_CTRL   0xA4
 
 #define CH341_REG_BREAK        0x05
+#define CH341_REG_PRESCALER    0x12
+#define CH341_REG_DIVISOR      0x13
 #define CH341_REG_LCR          0x18
+#define CH341_REG_LCR2         0x25
+
 #define CH341_NBREAK_BITS      0x01
 
 #define CH341_LCR_ENABLE_RX    0x80

@@ -246,11 +250,20 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 	 */
 	val |= BIT(7);
 
-	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x1312, val);
+	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
+			      CH341_REG_DIVISOR << 8 | CH341_REG_PRESCALER,
+			      val);
 	if (r)
 		return r;
 
-	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x2518, lcr);
+	/*
+	 * Chip versions before version 0x30 as read using
+	 * CH341_REQ_READ_VERSION used separate registers for line control
+	 * (word length, parity and word length). Version 0x30 and above use
+	 * CH341_REG_LCR only and CH341_REG_LCR2 is always set to zero.
+	 */
+	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
+			      CH341_REG_LCR2 << 8 | CH341_REG_LCR, lcr);
 	if (r)
 		return r;

Signature · 1 line

2.20.1

Johan Hovold

maintainer

2020-06-29 20:40 UTC #12 lore raw

Re: [PATCH v2 4/6] USB: serial: ch341: Name prescaler, divisor registers

Quoted 16 lines · Michael Hanselmann

Quoted 9 lines · Johan Hovold
Quoted 6 lines · Michael Hanselmann
The 0x25 register is only used by CH341 chips before version 0x30 and is involved in configuring the line control parameters. It's not known to the author whether there any such chips in the wild, and the driver never supported them (other registers are also treated differently). The alternative would've been to not set the register, but that may have unintended effects.
Quoted 5 lines
How did you come to those conclusions? I see this register being written the value zero in some older version of the vendor driver, but not in more recent ones.
Are you sure it's at all related to LCR?

Quoted 7 lines

I am looking at version 3.50.2014.8 from 2019-03-04 of CH341S64.SYS, the vendor driver. The function configuring registers 0x18 and 0x25 starts at offset 0x119f0.
For chip versions 0x30 and above 0x25 is always set to 0. For versions before 0x30 the values stored in register 0x25 depend on stop bit, partity and word length settings.

Great, thanks for confirming.

Quoted 43 lines

From aa597afe1e9cf5641b94b377ce63248c2d0d677a Mon Sep 17 00:00:00 2001
From: Michael Hanselmann <[email protected]>
Date: Wed, 27 May 2020 22:56:54 +0200
Subject: [PATCH] USB: serial: ch341: Name prescaler, divisor registers
Add constants for the prescaler and divisor registers. Document and name register 0x25, and put the LCR define to more use.
The 0x25 register (CH341_REG_LCR2) is only used by CH341 chips before version 0x30 and is involved in configuring the line control parameters. It's not known to the author whether there any such chips in the wild, and Linux' ch341 driver never supported them. For chip version 0x30 and above the 0x25 register is always set to zero. The alternative would've been to not set the register at all, but that may have unintended effects.
Signed-off-by Michael Hanselmann <[email protected]>
---
 drivers/usb/serial/ch341.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
drivers/usb/serial/ch341.c +5 -1
index 89675ee29645..97214e1ec5d2 100644
@@ -59,7 +59,11 @@
 #define CH341_REQ_MODEM_CTRL   0xA4
 
 #define CH341_REG_BREAK        0x05
+#define CH341_REG_PRESCALER    0x12
+#define CH341_REG_DIVISOR      0x13
 #define CH341_REG_LCR          0x18
+#define CH341_REG_LCR2         0x25
+
 #define CH341_NBREAK_BITS      0x01
 
 #define CH341_LCR_ENABLE_RX    0x80
@@ -246,11 +250,20 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 	 */
 	val |= BIT(7);
 
-	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x1312, val);
+	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,

drivers/usb/serial/ch341.c static int ch341_set_baudrate_lcr(struct usb_device *dev, · line 262

Quoted 10 lines

drivers/usb/serial/ch341.c +6 -1

+			      CH341_REG_DIVISOR << 8 | CH341_REG_PRESCALER,
+			      val);
 	if (r)
 		return r;
 
-	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x2518, lcr);
+	/*
+	 * Chip versions before version 0x30 as read using
+	 * CH341_REQ_READ_VERSION used separate registers for line control
+	 * (word length, parity and word length). Version 0x30 and above use

I replaced the first "word length" with "stop bits" here before applying.

drivers/usb/serial/ch341.c static int ch341_set_baudrate_lcr(struct usb_device *dev, · line 268

Quoted 6 lines

drivers/usb/serial/ch341.c +4 -0

+	 * CH341_REG_LCR only and CH341_REG_LCR2 is always set to zero.
+	 */
+	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
+			      CH341_REG_LCR2 << 8 | CH341_REG_LCR, lcr);
 	if (r)
 		return r;

Johan

Michael Hanselmann

author

2020-03-31 23:37 UTC #13 lore raw

PATCH v2 5/6 USB: serial: ch341: Compute minimum baud rate

In reply to Michael Hanselmann

The minimum baud rate was hardcoded, not computed. A patch later in the series will make use of the minimum baud rate as well.

The (1 + ((x - 1) / y)) pattern is to force rounding up (mathematically the minimum rate is about 45.78bps).

Signed-off-by Michael Hanselmann <[email protected]>

---
 drivers/usb/serial/ch341.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/usb/serial/ch341.c +5 -3

index 9407e12d9fbc..c820772e6a07 100644
@@ -147,6 +147,8 @@ static int ch341_control_in(struct usb_device *dev,
 
 #define CH341_CLKRATE		48000000
 #define CH341_CLK_DIV(ps, fact)	(1 << (12 - 3 * (ps) - (fact)))
+#define CH341_MIN_BPS \
+	(unsigned int)(1 + (((CH341_CLKRATE) - 1) / (CH341_CLK_DIV(0, 0) * 256)))
 #define CH341_MIN_RATE(ps, fact) \
 	(CH341_CLKRATE / (CH341_CLK_DIV((ps), (fact)) * 256))

@@ -210,10 +212,10 @@ static int ch341_get_divisor(struct ch341_private *priv)
 	speed = priv->baud_rate;
 
 	/*
-	 * Clamp to supported range, this makes the (ps < 0) and (div < 2)
-	 * sanity checks below redundant.
+	 * Clamp to supported range, making the later range sanity checks
+	 * redundant.
 	 */
-	speed = clamp(speed, 46U, 3000000U);
+	speed = clamp(speed, CH341_MIN_BPS, 3000000U);
 
 	if (priv->quirks & CH341_QUIRK_LIMITED_PRESCALER) {
 		/*

Signature · 1 line

2.20.1

Michael Hanselmann

author

2020-05-27 22:19 UTC #14 lore raw

Re: [PATCH v2 5/6] USB: serial: ch341: Compute minimum baud rate

The minimum baud rate was hardcoded, not computed from first principles. The break condition simulation added in a later patch will also need to make use of the minimum baud rate.

The (1 + ((x - 1) / y)) pattern is to force rounding up (mathematically the minimum rate is about 45.78bps).

Signed-off-by Michael Hanselmann <[email protected]>

---
Rebase on top of usb-next and wording change in commit message.

 drivers/usb/serial/ch341.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/usb/serial/ch341.c +5 -3

index 97214e1ec5d2..202cfa4ef6c7 100644
@@ -147,6 +147,8 @@ static int ch341_control_in(struct usb_device *dev,
 
 #define CH341_CLKRATE		48000000
 #define CH341_CLK_DIV(ps, fact)	(1 << (12 - 3 * (ps) - (fact)))
+#define CH341_MIN_BPS \
+	(unsigned int)(1 + (((CH341_CLKRATE) - 1) / (CH341_CLK_DIV(0, 0) * 256)))
 #define CH341_MIN_RATE(ps)	(CH341_CLKRATE / (CH341_CLK_DIV((ps), 1) * 512))
 
 static const speed_t ch341_min_rates[] = {

@@ -174,10 +176,10 @@ static int ch341_get_divisor(struct ch341_private *priv)
 	int ps;
 
 	/*
-	 * Clamp to supported range, this makes the (ps < 0) and (div < 2)
-	 * sanity checks below redundant.
+	 * Clamp to supported range, making the later range sanity checks
+	 * redundant.
 	 */
-	speed = clamp(speed, 46U, 3000000U);
+	speed = clamp(speed, CH341_MIN_BPS, 3000000U);
 
 	/*
 	 * Start with highest possible base clock (fact = 1) that will give a

Signature · 1 line

2.20.1

Johan Hovold

maintainer

2020-06-30 09:58 UTC #15 lore raw

Re: [PATCH v2 5/6] USB: serial: ch341: Compute minimum baud rate

Quoted 19 lines · Michael Hanselmann

The minimum baud rate was hardcoded, not computed from first principles. The break condition simulation added in a later patch will also need to make use of the minimum baud rate.
The (1 + ((x - 1) / y)) pattern is to force rounding up (mathematically the minimum rate is about 45.78bps).
Signed-off-by Michael Hanselmann <[email protected]>
---
Rebase on top of usb-next and wording change in commit message.
 drivers/usb/serial/ch341.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/usb/serial/ch341.c +0 -0
index 97214e1ec5d2..202cfa4ef6c7 100644

drivers/usb/serial/ch341.c static int ch341_control_in(struct usb_device *dev, · line 151

Quoted 5 lines

drivers/usb/serial/ch341.c +2 -0

@@ -147,6 +147,8 @@ static int ch341_control_in(struct usb_device *dev,
 
 #define CH341_CLKRATE		48000000
 #define CH341_CLK_DIV(ps, fact)	(1 << (12 - 3 * (ps) - (fact)))
+#define CH341_MIN_BPS \
+	(unsigned int)(1 + (((CH341_CLKRATE) - 1) / (CH341_CLK_DIV(0, 0) * 256)))

We have DIV_ROUND_UP(), and the cast can be avoided by using clamp_val().

drivers/usb/serial/ch341.c static int ch341_get_divisor(struct ch341_private *priv) · line 180

Quoted 11 lines

drivers/usb/serial/ch341.c +2 -2

 #define CH341_MIN_RATE(ps)	(CH341_CLKRATE / (CH341_CLK_DIV((ps), 1) * 512))
 
 static const speed_t ch341_min_rates[] = {

@@ -174,10 +176,10 @@ static int ch341_get_divisor(struct ch341_private *priv)
 	int ps;
 
 	/*
-	 * Clamp to supported range, this makes the (ps < 0) and (div < 2)
-	 * sanity checks below redundant.
+	 * Clamp to supported range, making the later range sanity checks
+	 * redundant.

This change seems uncalled for.

drivers/usb/serial/ch341.c static int ch341_get_divisor(struct ch341_private *priv) · line 185

Quoted 6 lines

drivers/usb/serial/ch341.c +1 -1

 	 */
-	speed = clamp(speed, 46U, 3000000U);
+	speed = clamp(speed, CH341_MIN_BPS, 3000000U);
 
 	/*
 	 * Start with highest possible base clock (fact = 1) that will give a

I replaced this patch with the below one adding both MIN and MAX macros instead.

Johan

From fd1d3198e26696ba66e8ac2111acadd360eb86b3 Mon Sep 17 00:00:00 2001
From: Johan Hovold <[email protected]>
Date: Mon, 29 Jun 2020 14:18:48 +0200
Subject: [PATCH] USB: serial: ch341: add min and max line-speed macros

The line-speed algorithm clamps the requested value to the supported range instead of bailing out on unsupported values.

Provide min and max macros and indicate how they are derived instead of hardcoding the limits.

Note that the algorithm depends on the minimum rate (45.78 bps) being rounded up (and the maximum rate being rounded down) to avoid special casing.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/serial/ch341.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/usb/serial/ch341.c +5 -1

index 684d595e7630..55a1c6dbeeb2 100644
@@ -156,6 +156,10 @@ static const speed_t ch341_min_rates[] = {
 	CH341_MIN_RATE(3),
 };
 
+/* Supported range is 46 to 3000000 bps. */
+#define CH341_MIN_BPS	DIV_ROUND_UP(CH341_CLKRATE, CH341_CLK_DIV(0, 0) * 256)
+#define CH341_MAX_BPS	(CH341_CLKRATE / (CH341_CLK_DIV(3, 0) * 2))
+
 /*
  * The device line speed is given by the following equation:
  *

@@ -177,7 +181,7 @@ static int ch341_get_divisor(struct ch341_private *priv)
 	 * Clamp to supported range, this makes the (ps < 0) and (div < 2)
 	 * sanity checks below redundant.
 	 */
-	speed = clamp(speed, 46U, 3000000U);
+	speed = clamp_val(speed, CH341_MIN_BPS, CH341_MAX_BPS);
 
 	/*
 	 * Start with highest possible base clock (fact = 1) that will give a

Signature · 1 line

2.26.2

Michael Hanselmann

author

2020-03-31 23:37 UTC #16 lore raw

PATCH v2 6/6 USB: serial: ch341: Simulate break condition if not supported

In reply to Michael Hanselmann

A subset of all CH341 devices don't support a real break condition. This fact is already used in the "ch341_detect_quirks" function. With this change a quirk is implemented to simulate a break condition by temporarily lowering the baud rate and sending a NUL byte.

The primary drawback of this approach is that the duration of the break can't be controlled by userland.

The "TTY_DRIVER_HARDWARE_BREAK" serial driver flag was investigated as an alternative. It's a driver-wide flag and would've required significant changes to the serial and USB-serial driver frameworks to expose it for individual USB-serial adapters.

Tested by sending a break condition and watching the TX pin using an oscilloscope.

Signed-off-by Michael Hanselmann <[email protected]>

---
 drivers/usb/serial/ch341.c | 95 ++++++++++++++++++++++++++++++++++----
 1 file changed, 85 insertions(+), 10 deletions(-)

drivers/usb/serial/ch341.c +85 ~~-10~~

index c820772e6a07..fc8ef816d143 100644
@@ -25,6 +25,7 @@
 #define DEFAULT_TIMEOUT   1000
 
 #define CH341_QUIRK_LIMITED_PRESCALER 0x01
+#define CH341_QUIRK_SIMULATE_BREAK    0x02
 
 /* flags for IO-Bits */
 #define CH341_BIT_RTS (1 << 6)

@@ -94,6 +95,7 @@ struct ch341_private {
 	u8 msr;
 	u8 lcr;
 	u8 quirks;
+	unsigned long break_end;
 };
 
 static void ch341_set_termios(struct tty_struct *tty,

@@ -203,14 +205,11 @@ static int ch341_calc_divisor(speed_t speed, unsigned int ps,
  *		2 <= div <= 256 if fact = 0, or
  *		9 <= div <= 256 if fact = 1
  */
-static int ch341_get_divisor(struct ch341_private *priv)
+static int ch341_get_divisor(struct ch341_private *priv, speed_t speed)
 {
-	speed_t speed;
 	unsigned int fact, div, clk_div;
 	int ps;
 
-	speed = priv->baud_rate;
-
 	/*
 	 * Clamp to supported range, making the later range sanity checks
 	 * redundant.

@@ -282,15 +281,16 @@ static int ch341_get_divisor(struct ch341_private *priv)
 }
 
 static int ch341_set_baudrate_lcr(struct usb_device *dev,
-				  struct ch341_private *priv, u8 lcr)
+				  struct ch341_private *priv,
+				  unsigned baud_rate, u8 lcr)
 {
 	int val;
 	int r;
 
-	if (!priv->baud_rate)
+	if (!baud_rate)
 		return -EINVAL;
 
-	val = ch341_get_divisor(priv);
+	val = ch341_get_divisor(priv, baud_rate);
 	if (val < 0)
 		return -EINVAL;

@@ -369,7 +369,7 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
 	if (r < 0)
 		goto out;
 
-	r = ch341_set_baudrate_lcr(dev, priv, priv->lcr);
+	r = ch341_set_baudrate_lcr(dev, priv, priv->baud_rate, priv->lcr);
 	if (r < 0)
 		goto out;

@@ -401,7 +401,7 @@ static int ch341_detect_quirks(struct usb_device *dev)
 	if (r == -EPIPE) {
 		dev_dbg(&dev->dev, "%s - reading break condition register"
 			" failed (%d)\n", __func__, r);
-		r = CH341_QUIRK_LIMITED_PRESCALER;
+		r = CH341_QUIRK_LIMITED_PRESCALER | CH341_QUIRK_SIMULATE_BREAK;
 		goto out;
 	}

@@ -579,7 +579,8 @@ static void ch341_set_termios(struct tty_struct *tty,
 	if (baud_rate) {
 		priv->baud_rate = baud_rate;
 
-		r = ch341_set_baudrate_lcr(port->serial->dev, priv, lcr);
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv,
+					   priv->baud_rate, lcr);
 		if (r < 0 && old_termios) {
 			priv->baud_rate = tty_termios_baud_rate(old_termios);
 			tty_termios_copy_hw(&tty->termios, old_termios);

@@ -598,15 +599,89 @@ static void ch341_set_termios(struct tty_struct *tty,
 	ch341_set_handshake(port->serial->dev, priv->mcr);
 }
 
+/*
+ * A subset of all CH34x devices don't support a real break condition and
+ * reading CH341_REG_BREAK fails (see also ch341_detect_quirks). This function
+ * simulates a break condition by lowering the baud rate to the minimum
+ * supported by the hardware upon enabling the break condition and sending
+ * a NUL byte.
+ *
+ * Normally the duration of the break condition can be controlled individually
+ * by userspace using TIOCSBRK and TIOCCBRK or by passing an argument to
+ * TCSBRKP. Due to how the simulation is implemented the duration can't be
+ * controlled. The duration is always 1s / 46bd * 10bit = 217ms.
+ */
+static void ch341_simulate_break(struct tty_struct *tty, int break_state)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct ch341_private *priv = usb_get_serial_port_data(port);
+	unsigned long delay;
+	int r;
+
+	if (break_state != 0) {
+		dev_dbg(&port->dev, "%s - Enter break state requested\n",
+			__func__);
+
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv,
+			CH341_MIN_BPS,
+			CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX |
+			CH341_LCR_CS8);
+		if (r < 0) {
+			dev_err(&port->dev, "%s - baud rate status %d\n",
+				__func__, r);
+			goto restore;
+		}
+
+		r = tty_put_char(tty, '\0');
+		if (r < 0) {
+			dev_err(&port->dev, "%s - write status %d\n",
+				__func__, r);
+			goto restore;
+		}
+
+		priv->break_end = jiffies + (10 * HZ / CH341_MIN_BPS);
+
+		return;
+	}
+
+	dev_dbg(&port->dev, "%s - Leave break state requested\n", __func__);
+
+	if (time_before(jiffies, priv->break_end)) {
+		/* Wait until NUL byte is written */
+		delay = min_t(unsigned long, HZ, priv->break_end - jiffies);
+
+		dev_dbg(&port->dev, "%s - sleep for %d ms\n", __func__,
+			jiffies_to_msecs(delay));
+		schedule_timeout_interruptible(delay);
+	}
+
+restore:
+	/* Restore original baud rate */
+	r = ch341_set_baudrate_lcr(port->serial->dev, priv, priv->baud_rate,
+				   priv->lcr);
+	if (r < 0)
+		dev_err(&port->dev, "%s - baud rate status %d\n", __func__, r);
+}
+
 static void ch341_break_ctl(struct tty_struct *tty, int break_state)
 {
 	const uint16_t ch341_break_reg =
 			((uint16_t) CH341_REG_LCR << 8) | CH341_REG_BREAK;
 	struct usb_serial_port *port = tty->driver_data;
+	struct ch341_private *priv = usb_get_serial_port_data(port);
 	int r;
 	uint16_t reg_contents;
 	uint8_t *break_reg;
 
+	if (priv->quirks & CH341_QUIRK_SIMULATE_BREAK) {
+		dev_warn_once(&port->dev,
+			      "%s - hardware doesn't support real break"
+			      " condition, simulating instead\n",
+			      __func__);
+		ch341_simulate_break(tty, break_state);
+		return;
+	}
+
 	break_reg = kmalloc(2, GFP_KERNEL);
 	if (!break_reg)
 		return;

Signature · 1 line

2.20.1

Johan Hovold

maintainer

2020-05-14 14:47 UTC #17 lore raw

Re: [PATCH v2 6/6] USB: serial: ch341: Simulate break condition if not supported

Quoted 58 lines · Michael Hanselmann

A subset of all CH341 devices don't support a real break condition. This fact is already used in the "ch341_detect_quirks" function. With this change a quirk is implemented to simulate a break condition by temporarily lowering the baud rate and sending a NUL byte.
The primary drawback of this approach is that the duration of the break can't be controlled by userland.
The "TTY_DRIVER_HARDWARE_BREAK" serial driver flag was investigated as an alternative. It's a driver-wide flag and would've required significant changes to the serial and USB-serial driver frameworks to expose it for individual USB-serial adapters.
Tested by sending a break condition and watching the TX pin using an oscilloscope.
Signed-off-by Michael Hanselmann <[email protected]>
---
 drivers/usb/serial/ch341.c | 95 ++++++++++++++++++++++++++++++++++----
 1 file changed, 85 insertions(+), 10 deletions(-)
drivers/usb/serial/ch341.c +3 -4
index c820772e6a07..fc8ef816d143 100644
@@ -25,6 +25,7 @@
 #define DEFAULT_TIMEOUT   1000
 
 #define CH341_QUIRK_LIMITED_PRESCALER 0x01
+#define CH341_QUIRK_SIMULATE_BREAK    0x02
 
 /* flags for IO-Bits */
 #define CH341_BIT_RTS (1 << 6)
@@ -94,6 +95,7 @@ struct ch341_private {
 	u8 msr;
 	u8 lcr;
 	u8 quirks;
+	unsigned long break_end;
 };
 
 static void ch341_set_termios(struct tty_struct *tty,
@@ -203,14 +205,11 @@ static int ch341_calc_divisor(speed_t speed, unsigned int ps,
  *		2 <= div <= 256 if fact = 0, or
  *		9 <= div <= 256 if fact = 1
  */
-static int ch341_get_divisor(struct ch341_private *priv)
+static int ch341_get_divisor(struct ch341_private *priv, speed_t speed)
 {
-	speed_t speed;
 	unsigned int fact, div, clk_div;
 	int ps;
 
-	speed = priv->baud_rate;
-
 	/*
 	 * Clamp to supported range, making the later range sanity checks
 	 * redundant.

drivers/usb/serial/ch341.c static int ch341_get_divisor(struct ch341_private *priv) · line 285

Quoted 6 lines

drivers/usb/serial/ch341.c +2 -1

@@ -282,15 +281,16 @@ static int ch341_get_divisor(struct ch341_private *priv)
 }
 
 static int ch341_set_baudrate_lcr(struct usb_device *dev,
-				  struct ch341_private *priv, u8 lcr)
+				  struct ch341_private *priv,
+				  unsigned baud_rate, u8 lcr)

Use speed_t.

Quoted 61 lines

drivers/usb/serial/ch341.c +22 -5

 {
 	int val;
 	int r;
 
-	if (!priv->baud_rate)
+	if (!baud_rate)
 		return -EINVAL;
 
-	val = ch341_get_divisor(priv);
+	val = ch341_get_divisor(priv, baud_rate);
 	if (val < 0)
 		return -EINVAL;

@@ -369,7 +369,7 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
 	if (r < 0)
 		goto out;
 
-	r = ch341_set_baudrate_lcr(dev, priv, priv->lcr);
+	r = ch341_set_baudrate_lcr(dev, priv, priv->baud_rate, priv->lcr);
 	if (r < 0)
 		goto out;

@@ -401,7 +401,7 @@ static int ch341_detect_quirks(struct usb_device *dev)
 	if (r == -EPIPE) {
 		dev_dbg(&dev->dev, "%s - reading break condition register"
 			" failed (%d)\n", __func__, r);
-		r = CH341_QUIRK_LIMITED_PRESCALER;
+		r = CH341_QUIRK_LIMITED_PRESCALER | CH341_QUIRK_SIMULATE_BREAK;
 		goto out;
 	}

@@ -579,7 +579,8 @@ static void ch341_set_termios(struct tty_struct *tty,
 	if (baud_rate) {
 		priv->baud_rate = baud_rate;
 
-		r = ch341_set_baudrate_lcr(port->serial->dev, priv, lcr);
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv,
+					   priv->baud_rate, lcr);
 		if (r < 0 && old_termios) {
 			priv->baud_rate = tty_termios_baud_rate(old_termios);
 			tty_termios_copy_hw(&tty->termios, old_termios);

@@ -598,15 +599,89 @@ static void ch341_set_termios(struct tty_struct *tty,
 	ch341_set_handshake(port->serial->dev, priv->mcr);
 }
 
+/*
+ * A subset of all CH34x devices don't support a real break condition and
+ * reading CH341_REG_BREAK fails (see also ch341_detect_quirks). This function
+ * simulates a break condition by lowering the baud rate to the minimum
+ * supported by the hardware upon enabling the break condition and sending
+ * a NUL byte.
+ *
+ * Normally the duration of the break condition can be controlled individually
+ * by userspace using TIOCSBRK and TIOCCBRK or by passing an argument to
+ * TCSBRKP. Due to how the simulation is implemented the duration can't be
+ * controlled. The duration is always 1s / 46bd * 10bit = 217ms.
+ */
+static void ch341_simulate_break(struct tty_struct *tty, int break_state)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct ch341_private *priv = usb_get_serial_port_data(port);

drivers/usb/serial/ch341.c static void ch341_set_termios(struct tty_struct *tty, · line 627

Quoted 10 lines

drivers/usb/serial/ch341.c +10 -0

+	unsigned long delay;
+	int r;
+
+	if (break_state != 0) {
+		dev_dbg(&port->dev, "%s - Enter break state requested\n",
+			__func__);
+
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv,
+			CH341_MIN_BPS,
+			CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX |

Hmm. This would corrupt incoming data as well during the break.

drivers/usb/serial/ch341.c static void ch341_set_termios(struct tty_struct *tty, · line 631

Quoted 4 lines

drivers/usb/serial/ch341.c +4 -0

+			CH341_LCR_CS8);
+		if (r < 0) {
+			dev_err(&port->dev, "%s - baud rate status %d\n",
+				__func__, r);

Spell out error messages instead of relying on __func__; "failed to change baudrate: %d\n"?

drivers/usb/serial/ch341.c static void ch341_set_termios(struct tty_struct *tty, · line 638

Quoted 7 lines

drivers/usb/serial/ch341.c +7 -0

+			goto restore;
+		}
+
+		r = tty_put_char(tty, '\0');
+		if (r < 0) {
+			dev_err(&port->dev, "%s - write status %d\n",
+				__func__, r);

ditto

drivers/usb/serial/ch341.c static void ch341_set_termios(struct tty_struct *tty, · line 651

Quoted 13 lines

drivers/usb/serial/ch341.c +13 -0

+			goto restore;
+		}
+
+		priv->break_end = jiffies + (10 * HZ / CH341_MIN_BPS);
+
+		return;
+	}
+
+	dev_dbg(&port->dev, "%s - Leave break state requested\n", __func__);
+
+	if (time_before(jiffies, priv->break_end)) {
+		/* Wait until NUL byte is written */
+		delay = min_t(unsigned long, HZ, priv->break_end - jiffies);

Looks like this can underflow if you're preempted after the check.

Quoted 19 lines

drivers/usb/serial/ch341.c +14 -0

+
+		dev_dbg(&port->dev, "%s - sleep for %d ms\n", __func__,
+			jiffies_to_msecs(delay));
+		schedule_timeout_interruptible(delay);
+	}
+
+restore:
+	/* Restore original baud rate */
+	r = ch341_set_baudrate_lcr(port->serial->dev, priv, priv->baud_rate,
+				   priv->lcr);
+	if (r < 0)
+		dev_err(&port->dev, "%s - baud rate status %d\n", __func__, r);
+}
+
 static void ch341_break_ctl(struct tty_struct *tty, int break_state)
 {
 	const uint16_t ch341_break_reg =
 			((uint16_t) CH341_REG_LCR << 8) | CH341_REG_BREAK;
 	struct usb_serial_port *port = tty->driver_data;

drivers/usb/serial/ch341.c static void ch341_set_termios(struct tty_struct *tty, · line 680

Quoted 10 lines

drivers/usb/serial/ch341.c +6 -0

+	struct ch341_private *priv = usb_get_serial_port_data(port);
 	int r;
 	uint16_t reg_contents;
 	uint8_t *break_reg;
 
+	if (priv->quirks & CH341_QUIRK_SIMULATE_BREAK) {
+		dev_warn_once(&port->dev,
+			      "%s - hardware doesn't support real break"
+			      " condition, simulating instead\n",
+			      __func__);

Don't break the string, and drop the __func__.

drivers/usb/serial/ch341.c static void ch341_set_termios(struct tty_struct *tty, · line 687

Quoted 7 lines

drivers/usb/serial/ch341.c +4 -0

+		ch341_simulate_break(tty, break_state);
+		return;
+	}
+
 	break_reg = kmalloc(2, GFP_KERNEL);
 	if (!break_reg)
 		return;

Johan

Michael Hanselmann

author

2020-05-27 22:21 UTC #18 lore raw

Re: [PATCH v2 6/6] USB: serial: ch341: Simulate break condition if not supported

Quoted 7 lines · Johan Hovold

Review
Quoted 4 lines · Michael Hanselmann
static int ch341_set_baudrate_lcr(struct usb_device *dev,
(quoted diff) +2 -1
-				  struct ch341_private *priv, u8 lcr)
+				  struct ch341_private *priv,
+				  unsigned baud_rate, u8 lcr)
Use speed_t.

Done. ch341_private.baud_rate and ch341_set_termios also use unsigned though. Should those also be changed to speed_t?

Quoted 5 lines

Review
Quoted 3 lines
(quoted diff) +3 -0
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv,
+			CH341_MIN_BPS,
+			CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX |
Hmm. This would corrupt incoming data as well during the break.

Yes, there's no way to avoid that. In my opinion being able to send a break signal for a serial console interrupt (SysRq) outweighs the corruption. Updated the comment on ch341_simulate_break to mention the caveat.

Quoted 7 lines

Review
Quoted 5 lines
(quoted diff) +5 -0
+	dev_dbg(&port->dev, "%s - Leave break state requested\n", __func__);
+
+	if (time_before(jiffies, priv->break_end)) {
+		/* Wait until NUL byte is written */
+		delay = min_t(unsigned long, HZ, priv->break_end - jiffies);
Looks like this can underflow if you're preempted after the check.

Moved the subtraction before the min_t macro to only evaluate it once.

Quoted 7 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	if (priv->quirks & CH341_QUIRK_SIMULATE_BREAK) {
+		dev_warn_once(&port->dev,
+			      "%s - hardware doesn't support real break"
+			      " condition, simulating instead\n",
+			      __func__);

Don't break the string, and drop the __func__.

Done, also for the other error messages you pointed out.

Michael

---
From 94fec46e814276491c9a027c5d3912b68deb9c55 Mon Sep 17 00:00:00 2001
From: Michael Hanselmann <[email protected]>
Date: Thu, 5 Mar 2020 01:50:35 +0100
Subject: [PATCH 2/2] USB: serial: ch341: Simulate break condition if not
supported

The primary drawbacks of this approach are that the duration of the break can't be controlled by userland and that data incoming during a simulated break is corrupted.

Tested by sending a break condition and watching the TX pin using an oscilloscope.

Signed-off-by Michael Hanselmann <[email protected]>

---
 drivers/usb/serial/ch341.c | 105 ++++++++++++++++++++++++++++++++++---
 1 file changed, 97 insertions(+), 8 deletions(-)

drivers/usb/serial/ch341.c +97 -8

index 202cfa4ef6c7..1e63310cfd9c 100644
@@ -78,6 +78,7 @@
 #define CH341_LCR_CS5          0x00
 
 #define CH341_QUIRK_LIMITED_PRESCALER	BIT(0)
+#define CH341_QUIRK_SIMULATE_BREAK	BIT(1)
 
 static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(0x4348, 0x5523) },

@@ -94,6 +95,7 @@ struct ch341_private {
 	u8 msr;
 	u8 lcr;
 	unsigned long quirks;
+	unsigned long break_end;
 };
 
 static void ch341_set_termios(struct tty_struct *tty,

@@ -168,10 +170,9 @@ static const speed_t ch341_min_rates[] = {
  *		2 <= div <= 256 if fact = 0, or
  *		9 <= div <= 256 if fact = 1
  */
-static int ch341_get_divisor(struct ch341_private *priv)
+static int ch341_get_divisor(struct ch341_private *priv, speed_t speed)
 {
 	unsigned int fact, div, clk_div;
-	speed_t speed = priv->baud_rate;
 	bool force_fact0 = false;
 	int ps;

@@ -234,15 +235,16 @@ static int ch341_get_divisor(struct ch341_private *priv)
 }
 
 static int ch341_set_baudrate_lcr(struct usb_device *dev,
-				  struct ch341_private *priv, u8 lcr)
+				  struct ch341_private *priv,
+				  speed_t baud_rate, u8 lcr)
 {
 	int val;
 	int r;
 
-	if (!priv->baud_rate)
+	if (!baud_rate)
 		return -EINVAL;
 
-	val = ch341_get_divisor(priv);
+	val = ch341_get_divisor(priv, baud_rate);
 	if (val < 0)
 		return -EINVAL;

@@ -322,7 +324,7 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
 	if (r < 0)
 		goto out;
 
-	r = ch341_set_baudrate_lcr(dev, priv, priv->lcr);
+	r = ch341_set_baudrate_lcr(dev, priv, priv->baud_rate, priv->lcr);
 	if (r < 0)
 		goto out;

@@ -356,7 +358,7 @@ static int ch341_detect_quirks(struct usb_serial_port *port)
 			    CH341_REG_BREAK, 0, buffer, size, DEFAULT_TIMEOUT);
 	if (r == -EPIPE) {
 		dev_dbg(&port->dev, "break control not supported\n");
-		quirks = CH341_QUIRK_LIMITED_PRESCALER;
+		quirks = CH341_QUIRK_LIMITED_PRESCALER | CH341_QUIRK_SIMULATE_BREAK;
 		r = 0;
 		goto out;
 	}

@@ -537,7 +539,8 @@ static void ch341_set_termios(struct tty_struct *tty,
 	if (baud_rate) {
 		priv->baud_rate = baud_rate;
 
-		r = ch341_set_baudrate_lcr(port->serial->dev, priv, lcr);
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv,
+					   priv->baud_rate, lcr);
 		if (r < 0 && old_termios) {
 			priv->baud_rate = tty_termios_baud_rate(old_termios);
 			tty_termios_copy_hw(&tty->termios, old_termios);

@@ -556,15 +559,101 @@ static void ch341_set_termios(struct tty_struct *tty,
 	ch341_set_handshake(port->serial->dev, priv->mcr);
 }
 
+/*
+ * A subset of all CH34x devices don't support a real break condition and
+ * reading CH341_REG_BREAK fails (see also ch341_detect_quirks). This function
+ * simulates a break condition by lowering the baud rate to the minimum
+ * supported by the hardware upon enabling the break condition and sending
+ * a NUL byte.
+ *
+ * Incoming data is corrupted while the break condition is being simulated.
+ *
+ * Normally the duration of the break condition can be controlled individually
+ * by userspace using TIOCSBRK and TIOCCBRK or by passing an argument to
+ * TCSBRKP. Due to how the simulation is implemented the duration can't be
+ * controlled. The duration is always about (1s / 46bd * 9bit) = 196ms.
+ */
+static void ch341_simulate_break(struct tty_struct *tty, int break_state)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct ch341_private *priv = usb_get_serial_port_data(port);
+	unsigned long delay;
+	int r;
+
+	if (break_state != 0) {
+		dev_dbg(&port->dev, "enter break state requested\n");
+
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv,
+			CH341_MIN_BPS,
+			CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX |
+			CH341_LCR_CS8);
+		if (r < 0) {
+			dev_err(&port->dev,
+				"failed to change baud rate to %u: %d\n",
+				CH341_MIN_BPS, r);
+			goto restore;
+		}
+
+		r = tty_put_char(tty, '\0');
+		if (r < 0) {
+			dev_err(&port->dev,
+				"failed to write NUL byte for simulated break condition: %d\n",
+				r);
+			goto restore;
+		}
+
+		/*
+		 * Compute how long transmission will take and add a bit of
+		 * safety margin.
+		 */
+		priv->break_end = jiffies + (10 * HZ / CH341_MIN_BPS);
+
+		return;
+	}
+
+	dev_dbg(&port->dev, "leave break state requested\n");
+
+	if (time_before(jiffies, priv->break_end)) {
+		/*
+		 * Wait until NUL byte is written and limit delay to one second
+		 * at most.
+		 */
+		delay = priv->break_end - jiffies;
+		delay = min_t(unsigned long, HZ, delay);
+
+		dev_dbg(&port->dev,
+			"wait %d ms while transmitting NUL byte at %u baud\n",
+			jiffies_to_msecs(delay), CH341_MIN_BPS);
+		schedule_timeout_interruptible(delay);
+	}
+
+restore:
+	/* Restore original baud rate */
+	r = ch341_set_baudrate_lcr(port->serial->dev, priv, priv->baud_rate,
+				   priv->lcr);
+	if (r < 0)
+		dev_err(&port->dev,
+			"restoring original baud rate of %u failed: %d\n",
+			priv->baud_rate, r);
+}
+
 static void ch341_break_ctl(struct tty_struct *tty, int break_state)
 {
 	const uint16_t ch341_break_reg =
 			((uint16_t) CH341_REG_LCR << 8) | CH341_REG_BREAK;
 	struct usb_serial_port *port = tty->driver_data;
+	struct ch341_private *priv = usb_get_serial_port_data(port);
 	int r;
 	uint16_t reg_contents;
 	uint8_t *break_reg;
 
+	if (priv->quirks & CH341_QUIRK_SIMULATE_BREAK) {
+		dev_warn_once(&port->dev,
+			      "hardware doesn't support real break condition, simulating instead\n");
+		ch341_simulate_break(tty, break_state);
+		return;
+	}
+
 	break_reg = kmalloc(2, GFP_KERNEL);
 	if (!break_reg)
 		return;

Signature · 1 line

2.20.1

Johan Hovold

maintainer

2020-06-30 11:39 UTC #19 lore raw

Re: [PATCH v2 6/6] USB: serial: ch341: Simulate break condition if not supported

Quoted 11 lines · Michael Hanselmann

Quoted 7 lines · Johan Hovold
drivers/usb/serial/ch341.c static int ch341_get_divisor(struct ch341_private *priv)
Quoted 4 lines · Michael Hanselmann
static int ch341_set_baudrate_lcr(struct usb_device *dev,
drivers/usb/serial/ch341.c +2 -1
-				  struct ch341_private *priv, u8 lcr)
+				  struct ch341_private *priv,
+				  unsigned baud_rate, u8 lcr)
Use speed_t.
Done. ch341_private.baud_rate and ch341_set_termios also use unsigned though. Should those also be changed to speed_t?

I'd just leave it for now.

Quoted 19 lines

Quoted 5 lines
drivers/usb/serial/ch341.c static void ch341_set_termios(struct tty_struct *tty, · line 588
Quoted 3 lines
drivers/usb/serial/ch341.c +3 -0
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv,
+			CH341_MIN_BPS,
+			CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX |
Hmm. This would corrupt incoming data as well during the break.
Yes, there's no way to avoid that. In my opinion being able to send a break signal for a serial console interrupt (SysRq) outweighs the corruption. Updated the comment on ch341_simulate_break to mention the caveat.
Quoted 7 lines
Review
Quoted 5 lines
(quoted diff) +5 -0
+	dev_dbg(&port->dev, "%s - Leave break state requested\n", __func__);
+
+	if (time_before(jiffies, priv->break_end)) {
+		/* Wait until NUL byte is written */
+		delay = min_t(unsigned long, HZ, priv->break_end - jiffies);
Looks like this can underflow if you're preempted after the check.

Quoted 1 line

Moved the subtraction before the min_t macro to only evaluate it once.

That's not sufficient; the problem is that jiffies may be updated after time_before().

Quoted 143 lines

Quoted 7 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	if (priv->quirks & CH341_QUIRK_SIMULATE_BREAK) {
+		dev_warn_once(&port->dev,
+			      "%s - hardware doesn't support real break"
+			      " condition, simulating instead\n",
+			      __func__);

Don't break the string, and drop the __func__.

Done, also for the other error messages you pointed out.

Michael

The primary drawbacks of this approach are that the duration of the break can't be controlled by userland and that data incoming during a simulated break is corrupted.

Tested by sending a break condition and watching the TX pin using an oscilloscope.

Signed-off-by Michael Hanselmann <[email protected]>

---
 drivers/usb/serial/ch341.c | 105 ++++++++++++++++++++++++++++++++++---
 1 file changed, 97 insertions(+), 8 deletions(-)

drivers/usb/serial/ch341.c +29 -8

index 202cfa4ef6c7..1e63310cfd9c 100644
@@ -78,6 +78,7 @@
 #define CH341_LCR_CS5          0x00
 
 #define CH341_QUIRK_LIMITED_PRESCALER	BIT(0)
+#define CH341_QUIRK_SIMULATE_BREAK	BIT(1)
 
 static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(0x4348, 0x5523) },

@@ -94,6 +95,7 @@ struct ch341_private {
 	u8 msr;
 	u8 lcr;
 	unsigned long quirks;
+	unsigned long break_end;
 };
 
 static void ch341_set_termios(struct tty_struct *tty,

@@ -168,10 +170,9 @@ static const speed_t ch341_min_rates[] = {
  *		2 <= div <= 256 if fact = 0, or
  *		9 <= div <= 256 if fact = 1
  */
-static int ch341_get_divisor(struct ch341_private *priv)
+static int ch341_get_divisor(struct ch341_private *priv, speed_t speed)
 {
 	unsigned int fact, div, clk_div;
-	speed_t speed = priv->baud_rate;
 	bool force_fact0 = false;
 	int ps;

@@ -234,15 +235,16 @@ static int ch341_get_divisor(struct ch341_private *priv)
 }
 
 static int ch341_set_baudrate_lcr(struct usb_device *dev,
-				  struct ch341_private *priv, u8 lcr)
+				  struct ch341_private *priv,
+				  speed_t baud_rate, u8 lcr)
 {
 	int val;
 	int r;
 
-	if (!priv->baud_rate)
+	if (!baud_rate)
 		return -EINVAL;
 
-	val = ch341_get_divisor(priv);
+	val = ch341_get_divisor(priv, baud_rate);
 	if (val < 0)
 		return -EINVAL;

@@ -322,7 +324,7 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
 	if (r < 0)
 		goto out;
 
-	r = ch341_set_baudrate_lcr(dev, priv, priv->lcr);
+	r = ch341_set_baudrate_lcr(dev, priv, priv->baud_rate, priv->lcr);
 	if (r < 0)
 		goto out;

@@ -356,7 +358,7 @@ static int ch341_detect_quirks(struct usb_serial_port *port)
 			    CH341_REG_BREAK, 0, buffer, size, DEFAULT_TIMEOUT);
 	if (r == -EPIPE) {
 		dev_dbg(&port->dev, "break control not supported\n");
-		quirks = CH341_QUIRK_LIMITED_PRESCALER;
+		quirks = CH341_QUIRK_LIMITED_PRESCALER | CH341_QUIRK_SIMULATE_BREAK;
 		r = 0;
 		goto out;
 	}

@@ -537,7 +539,8 @@ static void ch341_set_termios(struct tty_struct *tty,
 	if (baud_rate) {
 		priv->baud_rate = baud_rate;
 
-		r = ch341_set_baudrate_lcr(port->serial->dev, priv, lcr);
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv,
+					   priv->baud_rate, lcr);
 		if (r < 0 && old_termios) {
 			priv->baud_rate = tty_termios_baud_rate(old_termios);
 			tty_termios_copy_hw(&tty->termios, old_termios);

@@ -556,15 +559,101 @@ static void ch341_set_termios(struct tty_struct *tty,
 	ch341_set_handshake(port->serial->dev, priv->mcr);
 }
 
+/*
+ * A subset of all CH34x devices don't support a real break condition and
+ * reading CH341_REG_BREAK fails (see also ch341_detect_quirks). This function
+ * simulates a break condition by lowering the baud rate to the minimum
+ * supported by the hardware upon enabling the break condition and sending
+ * a NUL byte.
+ *
+ * Incoming data is corrupted while the break condition is being simulated.
+ *
+ * Normally the duration of the break condition can be controlled individually
+ * by userspace using TIOCSBRK and TIOCCBRK or by passing an argument to
+ * TCSBRKP. Due to how the simulation is implemented the duration can't be
+ * controlled. The duration is always about (1s / 46bd * 9bit) = 196ms.
+ */
+static void ch341_simulate_break(struct tty_struct *tty, int break_state)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct ch341_private *priv = usb_get_serial_port_data(port);

drivers/usb/serial/ch341.c static void ch341_set_termios(struct tty_struct *tty, · line 589

Quoted 10 lines

drivers/usb/serial/ch341.c +10 -0

+	unsigned long delay;
+	int r;
+
+	if (break_state != 0) {
+		dev_dbg(&port->dev, "enter break state requested\n");
+
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv,
+			CH341_MIN_BPS,
+			CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX |
+			CH341_LCR_CS8);

Continuation lines should be indented at least two tabs further.

And let's just merge the last two lines.

Quoted 9 lines

drivers/usb/serial/ch341.c +9 -0

+		if (r < 0) {
+			dev_err(&port->dev,
+				"failed to change baud rate to %u: %d\n",
+				CH341_MIN_BPS, r);
+			goto restore;
+		}
+
+		r = tty_put_char(tty, '\0');
+		if (r < 0) {

drivers/usb/serial/ch341.c static void ch341_set_termios(struct tty_struct *tty, · line 608

Quoted 10 lines

drivers/usb/serial/ch341.c +10 -0

+			dev_err(&port->dev,
+				"failed to write NUL byte for simulated break condition: %d\n",
+				r);
+			goto restore;
+		}
+
+		/*
+		 * Compute how long transmission will take and add a bit of
+		 * safety margin.
+		 */

Where's that margin?

drivers/usb/serial/ch341.c static void ch341_set_termios(struct tty_struct *tty, · line 620

Quoted 12 lines

drivers/usb/serial/ch341.c +12 -0

+		priv->break_end = jiffies + (10 * HZ / CH341_MIN_BPS);
+
+		return;
+	}
+
+	dev_dbg(&port->dev, "leave break state requested\n");
+
+	if (time_before(jiffies, priv->break_end)) {
+		/*
+		 * Wait until NUL byte is written and limit delay to one second
+		 * at most.
+		 */

So you could still be preempted here so that delay would wrap. Just store jiffies in a "now" temporary before the conditional?

drivers/usb/serial/ch341.c static void ch341_set_termios(struct tty_struct *tty, · line 622

Quoted 2 lines

drivers/usb/serial/ch341.c +2 -0

+		delay = priv->break_end - jiffies;
+		delay = min_t(unsigned long, HZ, delay);

And then you can skip the min_t() here.

Quoted 20 lines

drivers/usb/serial/ch341.c +17 -0

+
+		dev_dbg(&port->dev,
+			"wait %d ms while transmitting NUL byte at %u baud\n",
+			jiffies_to_msecs(delay), CH341_MIN_BPS);
+		schedule_timeout_interruptible(delay);
+	}
+
+restore:
+	/* Restore original baud rate */
+	r = ch341_set_baudrate_lcr(port->serial->dev, priv, priv->baud_rate,
+				   priv->lcr);
+	if (r < 0)
+		dev_err(&port->dev,
+			"restoring original baud rate of %u failed: %d\n",
+			priv->baud_rate, r);
+}
+
 static void ch341_break_ctl(struct tty_struct *tty, int break_state)
 {
 	const uint16_t ch341_break_reg =

drivers/usb/serial/ch341.c static void ch341_set_termios(struct tty_struct *tty, · line 652

Quoted 10 lines

drivers/usb/serial/ch341.c +4 -0

 			((uint16_t) CH341_REG_LCR << 8) | CH341_REG_BREAK;
 	struct usb_serial_port *port = tty->driver_data;
+	struct ch341_private *priv = usb_get_serial_port_data(port);
 	int r;
 	uint16_t reg_contents;
 	uint8_t *break_reg;
 
+	if (priv->quirks & CH341_QUIRK_SIMULATE_BREAK) {
+		dev_warn_once(&port->dev,
+			      "hardware doesn't support real break condition, simulating instead\n");

This should probably be moved to probe and quirk_detect() and be a dev_info (e.g. consider having two of these devices in a system).

drivers/usb/serial/ch341.c static void ch341_set_termios(struct tty_struct *tty, · line 659

Quoted 7 lines

drivers/usb/serial/ch341.c +4 -0

+		ch341_simulate_break(tty, break_state);
+		return;
+	}
+
 	break_reg = kmalloc(2, GFP_KERNEL);
 	if (!break_reg)
 		return;

Care to respin as a v4?

Johan

Michael Hanselmann

author

2020-07-04 18:25 UTC #20 lore raw

Re: [PATCH v2 6/6] USB: serial: ch341: Simulate break condition if not supported

Quoted 9 lines · Johan Hovold

Review
Quoted 4 lines · Michael Hanselmann
(quoted diff) +4 -0
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv,
+			CH341_MIN_BPS,
+			CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX |
+			CH341_LCR_CS8);
Continuation lines should be indented at least two tabs further.
And let's just merge the last two lines.

Done.

Quoted 6 lines

Review
Quoted 4 lines
(quoted diff) +4 -0
+		/*
+		 * Compute how long transmission will take and add a bit of
+		 * safety margin.
+		 */
Where's that margin?

That wasn't obvious indeed. It's the 10 instead of 9 in the calculation. Updated the comment: "[…] and add a single bit of safety margin (the actual transmission is 8 bits plus one stop bit)."

Quoted 7 lines

(quoted diff) +7 -0

+		priv->break_end = jiffies + (10 * HZ / CH341_MIN_BPS);
+
+		return;
+	}
+
+	dev_dbg(&port->dev, "leave break state requested\n");
+

Quoted 8 lines

Review
Quoted 5 lines
(quoted diff) +5 -0
+	if (time_before(jiffies, priv->break_end)) {
+		/*
+		 * Wait until NUL byte is written and limit delay to one second
+		 * at most.
+		 */
So you could still be preempted here so that delay would wrap. Just store jiffies in a "now" temporary before the conditional?

Done, thank you for the suggestion.

Quoted 6 lines

Review
Quoted 3 lines
(quoted diff) +3 -0
+	if (priv->quirks & CH341_QUIRK_SIMULATE_BREAK) {
+		dev_warn_once(&port->dev,
+			      "hardware doesn't support real break condition, simulating instead\n");
This should probably be moved to probe and quirk_detect() and be a dev_info (e.g. consider having two of these devices in a system).

Done.

Updated patch included below.

Michael

---
From 41b8b06d343a69541a357d8c9d6d0fe3f22610d6 Mon Sep 17 00:00:00 2001
Message-Id: <41b8b06d343a69541a357d8c9d6d0fe3f22610d6.1593887001.git.public@hansmi.ch>
From: Michael Hanselmann <[email protected]>
Date: Thu, 5 Mar 2020 01:50:35 +0100
Subject: [PATCH] USB: serial: ch341: Simulate break condition if not supported

The primary drawbacks of this approach are that the duration of the break can't be controlled by userland and that data incoming during a simulated break is corrupted.

Tested by sending a break condition and watching the TX pin using an oscilloscope.

Signed-off-by Michael Hanselmann <[email protected]>

---
 drivers/usb/serial/ch341.c | 101 +++++++++++++++++++++++++++++++++----
 1 file changed, 92 insertions(+), 9 deletions(-)

drivers/usb/serial/ch341.c +92 -9

index 55a1c6dbeeb2..0cb02d1bde02 100644
@@ -78,6 +78,7 @@
 #define CH341_LCR_CS5          0x00
 
 #define CH341_QUIRK_LIMITED_PRESCALER	BIT(0)
+#define CH341_QUIRK_SIMULATE_BREAK	BIT(1)
 
 static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(0x4348, 0x5523) },

@@ -94,6 +95,7 @@ struct ch341_private {
 	u8 msr;
 	u8 lcr;
 	unsigned long quirks;
+	unsigned long break_end;
 };
 
 static void ch341_set_termios(struct tty_struct *tty,

@@ -170,10 +172,9 @@ static const speed_t ch341_min_rates[] = {
  *		2 <= div <= 256 if fact = 0, or
  *		9 <= div <= 256 if fact = 1
  */
-static int ch341_get_divisor(struct ch341_private *priv)
+static int ch341_get_divisor(struct ch341_private *priv, speed_t speed)
 {
 	unsigned int fact, div, clk_div;
-	speed_t speed = priv->baud_rate;
 	bool force_fact0 = false;
 	int ps;

@@ -236,15 +237,16 @@ static int ch341_get_divisor(struct ch341_private *priv)
 }
 
 static int ch341_set_baudrate_lcr(struct usb_device *dev,
-				  struct ch341_private *priv, u8 lcr)
+				  struct ch341_private *priv,
+				  speed_t baud_rate, u8 lcr)
 {
 	int val;
 	int r;
 
-	if (!priv->baud_rate)
+	if (!baud_rate)
 		return -EINVAL;
 
-	val = ch341_get_divisor(priv);
+	val = ch341_get_divisor(priv, baud_rate);
 	if (val < 0)
 		return -EINVAL;

@@ -324,7 +326,7 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
 	if (r < 0)
 		goto out;
 
-	r = ch341_set_baudrate_lcr(dev, priv, priv->lcr);
+	r = ch341_set_baudrate_lcr(dev, priv, priv->baud_rate, priv->lcr);
 	if (r < 0)
 		goto out;

@@ -357,8 +359,8 @@ static int ch341_detect_quirks(struct usb_serial_port *port)
 			    USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
 			    CH341_REG_BREAK, 0, buffer, size, DEFAULT_TIMEOUT);
 	if (r == -EPIPE) {
-		dev_dbg(&port->dev, "break control not supported\n");
-		quirks = CH341_QUIRK_LIMITED_PRESCALER;
+		dev_info(&port->dev, "hardware doesn't support real break condition, using simulation instead\n");
+		quirks = CH341_QUIRK_LIMITED_PRESCALER | CH341_QUIRK_SIMULATE_BREAK;
 		r = 0;
 		goto out;
 	}

@@ -539,7 +541,8 @@ static void ch341_set_termios(struct tty_struct *tty,
 	if (baud_rate) {
 		priv->baud_rate = baud_rate;
 
-		r = ch341_set_baudrate_lcr(port->serial->dev, priv, lcr);
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv,
+					   priv->baud_rate, lcr);
 		if (r < 0 && old_termios) {
 			priv->baud_rate = tty_termios_baud_rate(old_termios);
 			tty_termios_copy_hw(&tty->termios, old_termios);

@@ -558,15 +561,95 @@ static void ch341_set_termios(struct tty_struct *tty,
 	ch341_set_handshake(port->serial->dev, priv->mcr);
 }
 
+/*
+ * A subset of all CH34x devices don't support a real break condition and
+ * reading CH341_REG_BREAK fails (see also ch341_detect_quirks). This function
+ * simulates a break condition by lowering the baud rate to the minimum
+ * supported by the hardware upon enabling the break condition and sending
+ * a NUL byte.
+ *
+ * Incoming data is corrupted while the break condition is being simulated.
+ *
+ * Normally the duration of the break condition can be controlled individually
+ * by userspace using TIOCSBRK and TIOCCBRK or by passing an argument to
+ * TCSBRKP. Due to how the simulation is implemented the duration can't be
+ * controlled. The duration is always about (1s / 46bd * 9bit) = 196ms.
+ */
+static void ch341_simulate_break(struct tty_struct *tty, int break_state)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct ch341_private *priv = usb_get_serial_port_data(port);
+	unsigned long now, delay;
+	int r;
+
+	if (break_state != 0) {
+		dev_dbg(&port->dev, "enter break state requested\n");
+
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv, CH341_MIN_BPS,
+					CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX | CH341_LCR_CS8);
+		if (r < 0) {
+			dev_err(&port->dev,
+				"failed to change baud rate to %u: %d\n",
+				CH341_MIN_BPS, r);
+			goto restore;
+		}
+
+		r = tty_put_char(tty, '\0');
+		if (r < 0) {
+			dev_err(&port->dev,
+				"failed to write NUL byte for simulated break condition: %d\n",
+				r);
+			goto restore;
+		}
+
+		/*
+		 * Compute expected transmission duration and add a single bit
+		 * of safety margin (the actual NUL byte transmission is 8 bits
+		 * plus one stop bit).
+		 */
+		priv->break_end = jiffies + (10 * HZ / CH341_MIN_BPS);
+
+		return;
+	}
+
+	dev_dbg(&port->dev, "leave break state requested\n");
+
+	now = jiffies;
+
+	if (time_before(now, priv->break_end)) {
+		/* Wait until NUL byte is written */
+		delay = priv->break_end - now;
+		dev_dbg(&port->dev,
+			"wait %d ms while transmitting NUL byte at %u baud\n",
+			jiffies_to_msecs(delay), CH341_MIN_BPS);
+		schedule_timeout_interruptible(delay);
+	}
+
+restore:
+	/* Restore original baud rate */
+	r = ch341_set_baudrate_lcr(port->serial->dev, priv, priv->baud_rate,
+				   priv->lcr);
+	if (r < 0)
+		dev_err(&port->dev,
+			"restoring original baud rate of %u failed: %d\n",
+			priv->baud_rate, r);
+}
+
 static void ch341_break_ctl(struct tty_struct *tty, int break_state)
 {
 	const uint16_t ch341_break_reg =
 			((uint16_t) CH341_REG_LCR << 8) | CH341_REG_BREAK;
 	struct usb_serial_port *port = tty->driver_data;
+	struct ch341_private *priv = usb_get_serial_port_data(port);
 	int r;
 	uint16_t reg_contents;
 	uint8_t *break_reg;
 
+	if (priv->quirks & CH341_QUIRK_SIMULATE_BREAK) {
+		ch341_simulate_break(tty, break_state);
+		return;
+	}
+
 	break_reg = kmalloc(2, GFP_KERNEL);
 	if (!break_reg)
 		return;

Signature · 1 line

2.20.1

Johan Hovold

maintainer

2020-07-06 09:31 UTC #21 lore raw

Re: [PATCH v2 6/6] USB: serial: ch341: Simulate break condition if not supported

Hi Michael,

Quoted 8 lines · Michael Hanselmann

Quoted 1 line · Johan Hovold
On Thu, May 28, 2020 at 12:21:11AM +0200, Michael Hanselmann wrote:
Updated patch included below.
Michael
---

When sending patches inline like this, try to avoid adding a (---) marker like this as it makes git-am discard the commit message when applying.

Quoted 20 lines

From 41b8b06d343a69541a357d8c9d6d0fe3f22610d6 Mon Sep 17 00:00:00 2001
Message-Id: <41b8b06d343a69541a357d8c9d6d0fe3f22610d6.1593887001.git.public@hansmi.ch>
From: Michael Hanselmann <[email protected]>
Date: Thu, 5 Mar 2020 01:50:35 +0100
Subject: [PATCH] USB: serial: ch341: Simulate break condition if not supported
A subset of all CH341 devices don't support a real break condition. This fact is already used in the "ch341_detect_quirks" function. With this change a quirk is implemented to simulate a break condition by temporarily lowering the baud rate and sending a NUL byte.
The primary drawbacks of this approach are that the duration of the break can't be controlled by userland and that data incoming during a simulated break is corrupted.
The "TTY_DRIVER_HARDWARE_BREAK" serial driver flag was investigated as an alternative. It's a driver-wide flag and would've required significant changes to the serial and USB-serial driver frameworks to expose it for individual USB-serial adapters.

Quoted 4 lines

Tested by sending a break condition and watching the TX pin using an oscilloscope.
Signed-off-by Michael Hanselmann <[email protected]>

Now applied with a slightly condensed probe info message.

Thanks,
Johan

Johan Hovold

maintainer

2020-05-14 14:02 UTC #22 lore raw

In reply to Michael Hanselmann

Hi Michel,

and sorry about the late feedback on this one.

Quoted 10 lines · Michael Hanselmann

A subset of CH341 devices does not support all features, namely the prescaler is limited to a reduced precision and there is no support for sending a RS232 break condition.
These devices can usually be identified by an imprint of "340" on the turquoise-colored plug. They're also sometimes called "HL340", but not all such devices are affected.
This series takes multiple steps to support the limited chips:

Quoted 7 lines

* Remove special cases from clock prescaler computation
* Detect limited devices by trying to read a register related to sending a break condition
* Amend clock prescaler computation to only use working values on limited chips
* Simulate an RS232 break condition by temporarily lowering the baud rate and sending a NUL byte

Good summary, and generally clean overall. I know you replied to my earlier comments mentioning what you had done for v2, but its good always include a short changelog here when resending a series.

Quoted 10 lines

Michael Hanselmann (6):
USB: serial: ch341: Reduce special cases in clock calculation
USB: serial: ch341: Add basis for quirk detection
USB: serial: ch341: Limit prescaler on quirky chips
USB: serial: ch341: Name prescaler, divisor registers
USB: serial: ch341: Compute minimum baud rate
USB: serial: ch341: Simulate break condition if not supported
 drivers/usb/serial/ch341.c | 294 ++++++++++++++++++++++++++++++-------
 1 file changed, 244 insertions(+), 50 deletions(-)

Johan