Message-ID: <7d3df04c482e71760ccc941469c99412b608c92b.camel@ew.tq-group.com>
Date: Mon, 20 Oct 2025 10:09:29 +0200
From: Matthias Schiffer <matthias.schiffer@...tq-group.com>
To: Uwe Kleine-König <u.kleine-koenig@...libre.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

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.

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.

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/

Powered by blists - more mailing lists