[PATCH] Proper RTS control when buffers fill

[PATCH] Proper RTS control when buffers fill

[Brant Merryman]

CP210x hardware disables auto-RTS but leaves auto-CTS when
in hardware flow control mode and UART on cp210x hardware
is disabled. This allows data to flow out, but new data
will not come into the port. When re-opening the port, if
auto-CTS is enabled on the cp210x, then auto-RTS must be
re-enabled in the driver.

Signed-off-by: Brant Merryman <brant.merryman@silabs.com>
---
 drivers/usb/serial/cp210x.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index bcceb4ad8be0..74c9f3822bd2 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -917,6 +917,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 	u32 baud;
 	u16 bits;
 	u32 ctl_hs;
+	u32 flow_repl;
 
 	cp210x_read_u32_reg(port, CP210X_GET_BAUDRATE, &baud);
 
@@ -1015,8 +1016,24 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 	cp210x_read_reg_block(port, CP210X_GET_FLOW, &flow_ctl,
 			sizeof(flow_ctl));
 	ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake);
+	flow_repl = le32_to_cpu(flow_ctl.ulFlowReplace);
 	if (ctl_hs & CP210X_SERIAL_CTS_HANDSHAKE) {
 		dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
+		/*
+		 * CP210x hardware disables auto-RTS but leaves auto-CTS when
+		 * in hardware flow control mode and UART on cp210x hardware
+		 * is disabled. This allows data to flow out, but new data
+		 * will not come into the port. When re-opening the port, if
+		 * auto-CTS is enabled on the cp210x, then auto-RTS must be
+		 * re-enabled in the driver.
+		 */
+		flow_repl &= ~CP210X_SERIAL_RTS_MASK;
+		flow_repl |= CP210X_SERIAL_RTS_SHIFT(CP210X_SERIAL_RTS_FLOW_CTL);
+
+		flow_ctl.ulControlHandshake = cpu_to_le32(ctl_hs);
+		flow_ctl.ulFlowReplace = cpu_to_le32(flow_repl);
+		cp210x_write_reg_block(port, CP210X_SET_FLOW, &flow_ctl, sizeof(flow_ctl));
+
 		cflag |= CRTSCTS;
 	} else {
 		dev_dbg(dev, "%s - flow control = NONE\n", __func__);
-- 

Re: [PATCH] Proper RTS control when buffers fill

[Johan Hovold]

On Wed, Dec 18, 2019 at 08:40:48PM +0000, Brant Merryman wrote:

You forgot to add the Subject prefix.

> CP210x hardware disables auto-RTS but leaves auto-CTS when
> in hardware flow control mode and UART on cp210x hardware
> is disabled. This allows data to flow out, but new data
> will not come into the port. When re-opening the port, if
> auto-CTS is enabled on the cp210x, then auto-RTS must be
> re-enabled in the driver.
> 
> Signed-off-by: Brant Merryman <brant.merryman@silabs.com>
> ---
>  drivers/usb/serial/cp210x.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index bcceb4ad8be0..74c9f3822bd2 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -917,6 +917,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
>  	u32 baud;
>  	u16 bits;
>  	u32 ctl_hs;
> +	u32 flow_repl;
>  
>  	cp210x_read_u32_reg(port, CP210X_GET_BAUDRATE, &baud);
>  
> @@ -1015,8 +1016,24 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
>  	cp210x_read_reg_block(port, CP210X_GET_FLOW, &flow_ctl,
>  			sizeof(flow_ctl));
>  	ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake);
> +	flow_repl = le32_to_cpu(flow_ctl.ulFlowReplace);

Move inside the if block (after the comment).

>  	if (ctl_hs & CP210X_SERIAL_CTS_HANDSHAKE) {
>  		dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
> +		/*
> +		 * CP210x hardware disables auto-RTS but leaves auto-CTS when
> +		 * in hardware flow control mode and UART on cp210x hardware
> +		 * is disabled. This allows data to flow out, but new data
> +		 * will not come into the port. When re-opening the port, if
> +		 * auto-CTS is enabled on the cp210x, then auto-RTS must be
> +		 * re-enabled in the driver.
> +		 */

Maybe drop the "allows data to flow out" bit, the uart has been disabled
after all, right? And RTS is asserted on open by the TTY layer anyway so
data should flow in once opened.

As I asked you before, could you be more specific about what state
RTS-line end up in when disabling the UART (e.g. 0x00: statically
inactive)?

> +		flow_repl &= ~CP210X_SERIAL_RTS_MASK;
> +		flow_repl |= CP210X_SERIAL_RTS_SHIFT(CP210X_SERIAL_RTS_FLOW_CTL);
> +
> +		flow_ctl.ulControlHandshake = cpu_to_le32(ctl_hs);

Again, this line is not needed.

> +		flow_ctl.ulFlowReplace = cpu_to_le32(flow_repl);
> +		cp210x_write_reg_block(port, CP210X_SET_FLOW, &flow_ctl, sizeof(flow_ctl));

You still need to break this line. 

> +
>  		cflag |= CRTSCTS;
>  	} else {
>  		dev_dbg(dev, "%s - flow control = NONE\n", __func__);

Johan

[PATCH] Proper RTS control when buffers fill

[Brant Merryman]

Enables usb generic functions for throttle/unthrottle to prevent USB data
loss. CP210x hardware disables RTS but leaves CTS when in hardware flow
control mode and port is closed. When re-opening the serial port, if CTS
is enabled, then RTS must be re-enabled inside the driver.

Signed-off-by: Brant Merryman <brant.merryman@silabs.com>
---
 drivers/usb/serial/cp210x.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index f5143eedbc48..fd54181e741b 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -272,6 +272,8 @@ static struct usb_serial_driver cp210x_device = {
 	.break_ctl		= cp210x_break_ctl,
 	.set_termios		= cp210x_set_termios,
 	.tx_empty		= cp210x_tx_empty,
+	.throttle		= usb_serial_generic_throttle,
+	.unthrottle		= usb_serial_generic_unthrottle,
 	.tiocmget		= cp210x_tiocmget,
 	.tiocmset		= cp210x_tiocmset,
 	.attach			= cp210x_attach,
@@ -915,6 +917,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 	u32 baud;
 	u16 bits;
 	u32 ctl_hs;
+	u32 flow_repl;
 
 	cp210x_read_u32_reg(port, CP210X_GET_BAUDRATE, &baud);
 
@@ -1013,8 +1016,24 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 	cp210x_read_reg_block(port, CP210X_GET_FLOW, &flow_ctl,
 			sizeof(flow_ctl));
 	ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake);
+	flow_repl = le32_to_cpu(flow_ctl.ulFlowReplace);
+	/* CP210x hardware disables RTS but leaves CTS when in hardware
+	 * flow control mode and port is closed.
+	 * This allows data to flow out, but new data will not come into
+	 * the port. When re-opening the port, if CTS is enabled, then RTS
+	 * must be re-enabled. in the driver
+	 */
 	if (ctl_hs & CP210X_SERIAL_CTS_HANDSHAKE) {
 		dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
+		flow_repl &= ~CP210X_SERIAL_RTS_MASK;
+		flow_repl |= CP210X_SERIAL_RTS_SHIFT(
+			CP210X_SERIAL_RTS_FLOW_CTL);
+
+		flow_ctl.ulControlHandshake = cpu_to_le32(ctl_hs);
+		flow_ctl.ulFlowReplace = cpu_to_le32(flow_repl);
+		cp210x_write_reg_block(port, CP210X_SET_FLOW,
+			&flow_ctl, sizeof(flow_ctl));
+
 		cflag |= CRTSCTS;
 	} else {
 		dev_dbg(dev, "%s - flow control = NONE\n", __func__);
-- 

Re: [PATCH] Proper RTS control when buffers fill

[Johan Hovold]

First of all, please use the common "USB: serial: cp210x: " prefix for
your changes.

On Mon, Dec 09, 2019 at 07:17:26PM +0000, Brant Merryman wrote:
> Enables usb generic functions for throttle/unthrottle to prevent USB data
> loss. CP210x hardware disables RTS but leaves CTS when in hardware flow
> control mode and port is closed. When re-opening the serial port, if CTS
> is enabled, then RTS must be re-enabled inside the driver.

This took a while to parse, but makes a little more sense after looking
at the code and remembering that cp210x unlike other drivers is fetching
the termios settings from the device on every open rather than simply
reinitialising the device. Perhaps we should change that at some point.  

Please rephrase the above to say "auto-RTS" ("auto-CTS") so it doesn't
sound like we're controlling the state of the CTS input line (which
obviously makes no sense). Same for the comment in the code below.

I also think you should split this in two patches since these are
strictly two distinct issues; the first one fixing the auto-RTS settings
after reopening the port, and the second adding the throttle callbacks
so that the device can tell when the application can't keep up.

> Signed-off-by: Brant Merryman <brant.merryman@silabs.com>
> ---
>  drivers/usb/serial/cp210x.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index f5143eedbc48..fd54181e741b 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -272,6 +272,8 @@ static struct usb_serial_driver cp210x_device = {
>  	.break_ctl		= cp210x_break_ctl,
>  	.set_termios		= cp210x_set_termios,
>  	.tx_empty		= cp210x_tx_empty,
> +	.throttle		= usb_serial_generic_throttle,
> +	.unthrottle		= usb_serial_generic_unthrottle,
>  	.tiocmget		= cp210x_tiocmget,
>  	.tiocmset		= cp210x_tiocmset,
>  	.attach			= cp210x_attach,
> @@ -915,6 +917,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
>  	u32 baud;
>  	u16 bits;
>  	u32 ctl_hs;
> +	u32 flow_repl;
>  
>  	cp210x_read_u32_reg(port, CP210X_GET_BAUDRATE, &baud);
>  
> @@ -1013,8 +1016,24 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
>  	cp210x_read_reg_block(port, CP210X_GET_FLOW, &flow_ctl,
>  			sizeof(flow_ctl));
>  	ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake);
> +	flow_repl = le32_to_cpu(flow_ctl.ulFlowReplace);
> +	/* CP210x hardware disables RTS but leaves CTS when in hardware
> +	 * flow control mode and port is closed.
> +	 * This allows data to flow out, but new data will not come into
> +	 * the port. When re-opening the port, if CTS is enabled, then RTS
> +	 * must be re-enabled. in the driver
> +	 */

This isn't strictly true since we assert RTS in cp210x_dtr_rts() after
open() (but auto-RTS would incorrectly be disabled).

Also can you be a bit more specific here; is it the interface-disable
call in close() that unconditionally sets the RTS mode to "statically
inactive"?

Nits: Please fix the full-stop in the last sentence and use the right
format for multi-line comments, which is:

	/*
	 * blah, blah
	 */

Perhaps move the comment inside the if block (after dev_dbg()) as well.

>  	if (ctl_hs & CP210X_SERIAL_CTS_HANDSHAKE) {
>  		dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
> +		flow_repl &= ~CP210X_SERIAL_RTS_MASK;
> +		flow_repl |= CP210X_SERIAL_RTS_SHIFT(
> +			CP210X_SERIAL_RTS_FLOW_CTL);

Indent continuation lines at least two tabs further, but here I'd
probably just break the 80-column rule if that's even an issue.

> +
> +		flow_ctl.ulControlHandshake = cpu_to_le32(ctl_hs);

Not needed, you're only changing flow_repl.

> +		flow_ctl.ulFlowReplace = cpu_to_le32(flow_repl);
> +		cp210x_write_reg_block(port, CP210X_SET_FLOW,
> +			&flow_ctl, sizeof(flow_ctl));

At least two tabs here too.

> +
>  		cflag |= CRTSCTS;
>  	} else {
>  		dev_dbg(dev, "%s - flow control = NONE\n", __func__);

Johan