Re: [PATCH] serial: imx: allow CRTSCTS with RTS/CTS GPIOs

On Tue, 2025-10-21 at 10:59 +0200, Uwe Kleine-König wrote:

Hello Matthias,

On Mon, Oct 20, 2025 at 10:09:29AM +0200, Matthias Schiffer wrote:

quoted

On Fri, 2025-10-17 at 17:01 +0200, Uwe Kleine-König wrote:

quoted

On Thu, Oct 16, 2025 at 01:37:30PM +0200, Matthias Schiffer wrote:

quoted

The CTS GPIO is only evaluated when the CRTSCTS termios flag is enabled;
it should be possible to enable the flag when only GPIO and no hardware-
controlled RTS/CTS are available. UCR2_IRTS is kept enabled in this case,
so the hardware CTS is ignored.

Fixes: 58362d5be352 ("serial: imx: implement handshaking using gpios with the mctrl_gpio helper")
Signed-off-by: Matthias Schiffer <redacted>
---
 drivers/tty/serial/imx.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 500dfc009d03e..4a54a689a0603 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c

@@ -1117,8 +1117,8 @@ static void imx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 			ucr2 |= UCR2_CTS;
 			/*
 			 * UCR2_IRTS is unset if and only if the port is
-			 * configured for CRTSCTS, so we use inverted UCR2_IRTS
-			 * to get the state to restore to.
+			 * configured for hardware-controlled CRTSCTS, so we use
+			 * inverted UCR2_IRTS to get the state to restore to.
 			 */
 			if (!(ucr2 & UCR2_IRTS))
 				ucr2 |= UCR2_CTSC;

@@ -1780,7 +1780,7 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	if ((termios->c_cflag & CSIZE) == CS8)
 		ucr2 |= UCR2_WS;
 
-	if (!sport->have_rtscts)
+	if (!sport->have_rtscts && !sport->have_rtsgpio)
 		termios->c_cflag &= ~CRTSCTS;
 
 	if (port->rs485.flags & SER_RS485_ENABLED) {

This hunk makes sense.

quoted

@@ -1794,7 +1794,7 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 		else
 			imx_uart_rts_inactive(sport, &ucr2);
 
-	} else if (termios->c_cflag & CRTSCTS) {
+	} else if ((termios->c_cflag & CRTSCTS) && sport->have_rtscts) {

I agree to add the parens here and consider this more readable than the
alternative

	} else if (termios->c_cflag & CRTSCTS && sport->have_rtscts) {

. Note there is no real win here. If the port doesn't have RTS/CTS it
doesn't matter if it tries to control the RTS line. While you could
argue it shouldn't set the line, it only makes an externally observable
difference if one of the SoC's pads is muxed to its RTS function.
I claim it's more robust in this case (i.e. no uart-has-rtscts property
but a pinmux for the RTS line) to control the line according to the RTS
setting. This is (at least IMO) better and more expected than driving
this line to a constant level. So I oppose to this hunk.

quoted

 		/*
 		 * Only let receiver control RTS output if we were not requested
 		 * to have RTS inactive (which then should take precedence).

@@ -1803,7 +1803,7 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 			ucr2 |= UCR2_CTSC;
 	}
 
-	if (termios->c_cflag & CRTSCTS)
+	if ((termios->c_cflag & CRTSCTS) && sport->have_rtscts)
 		ucr2 &= ~UCR2_IRTS;
 	if (termios->c_cflag & CSTOPB)
 		ucr2 |= UCR2_STPB;

Hmm, not sure. On one hand the same argument applies as above, but on
the other if there are pins that are not explicitly configured but still
in their CTS function this might affect operation in a bad way.
Also this affects the (very usual) configuration where only RX, TX and
RTS are used and CTS is not. In this case have_rtscts is true (right?)
and then if there is an accidental CTS pin this is bad and not fixed by
your change. Hmmm...

I think it makes sense to always keep UCR2_IRTS set when have_rtscts is unset,
as otherwise there might be two separate CTS signals in the accidental CTS pin
case - the hardware + the GPIO one, both affecting the UART operation.

With that change you break setups that have an RTS-GPIO but rely on the
HW pin for the CTS function. Not sure how common that is, but in this
case you only want the first code change. You could argue that in that
case have_rtscts should be set, but that's somewhat fuzzy.

Such a setup should set have_rtscts IMO. In any case, my patch would not break
existing setups, as the CRTSCTS flag simply cannot be set for !have_rtscts
without these changes.

quoted

If we keep this change (the 3rd), the 2nd should also be included for
consistency in the code path where I just changed a comment - there, UCR2_CTSC
is set only when UCR2_IRTS is unset. The 2nd and 3rd change together keep
imx_uart_set_mctrl and imx_uart_set_termios aligned.

quoted

So in sum the 2nd and 3rd code change is controversial. If the first one
already fixes the problem you're facing, I suggest to go for only that.
If you still think that the 3rd (and maybe even the 2nd) change is a
good idea, I'd request to do that in a separate commit as this is a
separate problem. Also the commit log only describes the first change,
doesn't it?

The commit message describes the first and third change; the second is included
to keep the setup consistent. I don't think these changes can be separated well
- the second and third change only affect a case that couldn't occur without the
first (as (termios->c_cflag & CRTSCTS) && !sport->have_rtscts would never have
been true). My suggestion would be that I extend the commit message to explain
each change in detail.

I'd still request to split the patch in at least two patches. The first
code change is to allow rts-gpios to work at all. The two later patches
change details about how HW pins are controlled in the presence of
rts-gpios

Okay, will do.

Best,
Matthias

Best regards
Uwe

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/