Message-ID: <lgse44as4k6fpzarztfnfl7wbxq2bfg5k7m7l6xlsyx23pmem4@khal3tytgwjn>
Date: Tue, 21 Oct 2025 10:59:01 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Matthias Schiffer <matthias.schiffer@...tq-group.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Jiri Slaby <jirislaby@...nel.org>, Shawn Guo <shawnguo@...nel.org>, 
	Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>, 
	Fabio Estevam <festevam@...il.com>, linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org, 
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org, linux@...tq-group.com
Subject: Re: [PATCH] serial: imx: allow CRTSCTS with RTS/CTS GPIOs

Hello Matthias,

On Mon, Oct 20, 2025 at 10:09:29AM +0200, Matthias Schiffer wrote:
> On Fri, 2025-10-17 at 17:01 +0200, Uwe Kleine-König wrote:
> > On Thu, Oct 16, 2025 at 01:37:30PM +0200, Matthias Schiffer wrote:
> > > 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 <matthias.schiffer@...tq-group.com>
> > > ---
> > >  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.
> > 
> > > @@ -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.
> > 
> > >  		/*
> > >  		 * 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.

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

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists