Message-ID: <1963351b2e50c537418293e6ab9293576a239c98.camel@ew.tq-group.com>
Date: Tue, 21 Oct 2025 11:37:37 +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 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:
> > 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.

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.

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

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/

Powered by blists - more mailing lists