[PATCH v2 0/6] USB: serial: ch341: Add support for limited chips (was: Add support for HL340 devices)

[PATCH v2 0/6] USB: serial: ch341: Add support for limited chips (was: Add support for HL340 devices)

[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:

* 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(-)

-- 
2.20.1

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

[Michael Hanselmann]

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 <public@hansmi.ch>
---
 drivers/usb/serial/ch341.c | 90 ++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index c5ecdcd51ffc..85e7125d0194 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -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;
 }
 
-- 
2.20.1

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

[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 <public@hansmi.ch>
---
 drivers/usb/serial/ch341.c | 49 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 85e7125d0194..9c839f67c3d4 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -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;
 
-- 
2.20.1

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

[Johan Hovold]

On Tue, Mar 31, 2020 at 11:37:18PM +0000, Michael Hanselmann wrote:
> 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 <public@hansmi.ch>

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

> ---
>  drivers/usb/serial/ch341.c | 49 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 85e7125d0194..9c839f67c3d4 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -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.

>  };
>  
>  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);

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).

> +		r = 0;
> +		goto out;
> +	}
> +
> +	if (r < 0) {

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

> +		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;

And finally I moved this bit into the helper itself.

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

Johan

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

[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 <public@hansmi.ch>
---
 drivers/usb/serial/ch341.c | 70 ++++++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 9c839f67c3d4..67a5d4c3df42 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -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;
 	}
 
-- 
2.20.1

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

[Johan Hovold]

On Tue, Mar 31, 2020 at 11:37:19PM +0000, Michael Hanselmann wrote:
> 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 <public@hansmi.ch>
> ---
>  drivers/usb/serial/ch341.c | 70 ++++++++++++++++++++++++++++----------
>  1 file changed, 52 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 9c839f67c3d4..67a5d4c3df42 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -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;
>  	}

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 <johan@kernel.org>
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 <stable@vger.kernel.org>	# 5.5
Reported-by: Michael Hanselmann <public@hansmi.ch>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ch341.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index f2b93f4554a7..89675ee29645 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -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;
 	}
-- 
2.26.2

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

[Johan Hovold]

On Thu, May 14, 2020 at 04:17:43PM +0200, Johan Hovold wrote:
> 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 <johan@kernel.org>
> 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 <stable@vger.kernel.org>	# 5.5
> Reported-by: Michael Hanselmann <public@hansmi.ch>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/serial/ch341.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index f2b93f4554a7..89675ee29645 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -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;
>  	}

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

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

[Michael Hanselmann]

On 27.05.20 15:16, Johan Hovold wrote:
> 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

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

[Johan Hovold]

On Wed, May 27, 2020 at 05:41:56PM +0200, Michael Hanselmann wrote:
> On 27.05.20 15:16, Johan Hovold wrote:
> > 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

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

[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 <public@hansmi.ch>
---
 drivers/usb/serial/ch341.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 67a5d4c3df42..9407e12d9fbc 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -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;
 
-- 
2.20.1

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

[Johan Hovold]

On Tue, Mar 31, 2020 at 11:37:20PM +0000, Michael Hanselmann wrote:
> 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).

> 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?

> Signed-off-by: Michael Hanselmann <public@hansmi.ch>
> ---
>  drivers/usb/serial/ch341.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 67a5d4c3df42..9407e12d9fbc 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -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);

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

>  	if (r)
>  		return r;

Johan

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

[Michael Hanselmann]

On 14.05.20 16:24, Johan Hovold wrote:
> On Tue, Mar 31, 2020 at 11:37:20PM +0000, Michael Hanselmann wrote:
>> 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!

>> 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?

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.

>> -	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 <public@hansmi.ch>
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 <public@hansmi.ch>
---
 drivers/usb/serial/ch341.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 89675ee29645..97214e1ec5d2 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -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;
 
-- 
2.20.1

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

[Johan Hovold]

On Wed, May 27, 2020 at 10:59:53PM +0200, Michael Hanselmann wrote:
> On 14.05.20 16:24, Johan Hovold wrote:
> > On Tue, Mar 31, 2020 at 11:37:20PM +0000, Michael Hanselmann wrote:

> >> 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?
> 
> 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.

> From aa597afe1e9cf5641b94b377ce63248c2d0d677a Mon Sep 17 00:00:00 2001
> From: Michael Hanselmann <public@hansmi.ch>
> 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 <public@hansmi.ch>
> ---
>  drivers/usb/serial/ch341.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 89675ee29645..97214e1ec5d2 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -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

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

> +	 * 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

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

[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 <public@hansmi.ch>
---
 drivers/usb/serial/ch341.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 9407e12d9fbc..c820772e6a07 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -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) {
 		/*
-- 
2.20.1

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

[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 <public@hansmi.ch>
---
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(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 97214e1ec5d2..202cfa4ef6c7 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -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
-- 
2.20.1

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

[Johan Hovold]

On Thu, May 28, 2020 at 12:19:30AM +0200, Michael Hanselmann wrote:
> 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 <public@hansmi.ch>
> ---
> 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(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 97214e1ec5d2..202cfa4ef6c7 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -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().

>  #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.

>  	 */
> -	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 <johan@kernel.org>
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.

Suggested-by: Michael Hanselmann <public@hansmi.ch>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ch341.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 684d595e7630..55a1c6dbeeb2 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -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
-- 
2.26.2

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

[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 <public@hansmi.ch>
---
 drivers/usb/serial/ch341.c | 95 ++++++++++++++++++++++++++++++++++----
 1 file changed, 85 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index c820772e6a07..fc8ef816d143 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -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;
-- 
2.20.1

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

[Johan Hovold]

On Tue, Mar 31, 2020 at 11:37:22PM +0000, Michael Hanselmann wrote:
> 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 <public@hansmi.ch>
> ---
>  drivers/usb/serial/ch341.c | 95 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 85 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index c820772e6a07..fc8ef816d143 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -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)

Use speed_t.

>  {
>  	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 |

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

> +			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"?

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

ditto

> +			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.

> +
> +		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__);

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

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

Johan

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

[Michael Hanselmann]

On 14.05.20 16:47, Johan Hovold wrote:
> On Tue, Mar 31, 2020 at 11:37:22PM +0000, Michael Hanselmann wrote:
>>  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.

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

>> +		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.

>> +	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.

>> +	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 <public@hansmi.ch>
Date: Thu, 5 Mar 2020 01:50:35 +0100
Subject: [PATCH 2/2] 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.

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

Signed-off-by: Michael Hanselmann <public@hansmi.ch>
---
 drivers/usb/serial/ch341.c | 105 ++++++++++++++++++++++++++++++++++---
 1 file changed, 97 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 202cfa4ef6c7..1e63310cfd9c 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -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;
-- 
2.20.1

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

[Johan Hovold]

On Thu, May 28, 2020 at 12:21:11AM +0200, Michael Hanselmann wrote:
> On 14.05.20 16:47, Johan Hovold wrote:
> > On Tue, Mar 31, 2020 at 11:37:22PM +0000, Michael Hanselmann wrote:
> >>  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.
> 
> 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.

> >> +		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.
> 
> >> +	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.

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

> >> +	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 <public@hansmi.ch>
> Date: Thu, 5 Mar 2020 01:50:35 +0100
> Subject: [PATCH 2/2] 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.
> 
> Tested by sending a break condition and watching the TX pin using an
> oscilloscope.
> 
> Signed-off-by: Michael Hanselmann <public@hansmi.ch>
> ---
>  drivers/usb/serial/ch341.c | 105 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 97 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 202cfa4ef6c7..1e63310cfd9c 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -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);

Continuation lines should be indented at least two tabs further.

And let's just merge the last two lines.

> +		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.
> +		 */

Where's that 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.
> +		 */

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

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

And then you can skip the min_t() here.

> +
> +		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");

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).

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

Care to respin as a v4?

Johan

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

[Michael Hanselmann]

On 30.06.20 13:39, Johan Hovold wrote:
> On Thu, May 28, 2020 at 12:21:11AM +0200, Michael Hanselmann wrote:
>> +		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.

>> +		/*
>> +		 * 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)."

>> +		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?

Done, thank you for the suggestion.

>> +	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 <public@hansmi.ch>
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.

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

Signed-off-by: Michael Hanselmann <public@hansmi.ch>
---
 drivers/usb/serial/ch341.c | 101 +++++++++++++++++++++++++++++++++----
 1 file changed, 92 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 55a1c6dbeeb2..0cb02d1bde02 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -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;
-- 
2.20.1

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

[Johan Hovold]

Hi Michael,

On Sat, Jul 04, 2020 at 08:25:03PM +0200, Michael Hanselmann wrote:
> On 30.06.20 13:39, Johan Hovold wrote:
> > 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.

> From 41b8b06d343a69541a357d8c9d6d0fe3f22610d6 Mon Sep 17 00:00:00 2001
> Message-Id: <41b8b06d343a69541a357d8c9d6d0fe3f22610d6.1593887001.git.public@hansmi.ch>
> From: Michael Hanselmann <public@hansmi.ch>
> 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.
> 
> Tested by sending a break condition and watching the TX pin using an
> oscilloscope.
> 
> Signed-off-by: Michael Hanselmann <public@hansmi.ch>

Now applied with a slightly condensed probe info message.

Thanks,
Johan

Re: [PATCH v2 0/6] USB: serial: ch341: Add support for limited chips (was: Add support for HL340 devices)

[Johan Hovold]

Hi Michel,

and sorry about the late feedback on this one.

On Tue, Mar 31, 2020 at 11:37:16PM +0000, Michael Hanselmann wrote:
> 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

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.

> 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