Re: [PATCH v6 2/3] serial: imx: set_mctrl(): correctly restore autoRTS state

From: Sergey Organov <sorganov@gmail•com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix•de>
Cc: linux-serial@vger•kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation•org>,
	Sascha Hauer <s.hauer@pengutronix•de>,
	NXP Linux Team <linux-imx@nxp•com>,
	Pengutronix Kernel Team <kernel@pengutronix•de>,
	linux-arm-kernel@lists•infradead.org
Subject: Re: [PATCH v6 2/3] serial: imx: set_mctrl(): correctly restore autoRTS state
Date: Tue, 23 Jul 2019 12:20:38 +0300	[thread overview]
Message-ID: <87h87d1509.fsf@osv.gnss.ru> (raw)
In-Reply-To: <20190722202406.hr74mg64sxoovah7@pengutronix.de> ("Uwe \=\?utf-8\?Q\?Kleine-K\=C3\=B6nig\=22's\?\= message of "Mon, 22 Jul 2019 22:24:06 +0200")

Uwe Kleine-König <u.kleine-koenig@pengutronix•de> writes:

> On Mon, Jul 22, 2019 at 10:22:10PM +0300, Sergey Organov wrote:
>> imx_uart_set_mctrl() happened to set UCR2_CTSC bit whenever TIOCM_RTS
>> was set, no matter if RTS/CTS handshake is enabled or not. Now fixed by
>> turning handshake on only when CRTSCTS bit for the port is set.
>>
>> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix•de>
>> Reviewed-by: Sascha Hauer <s.hauer@pengutronix•de>
>> Tested-by: Sascha Hauer <s.hauer@pengutronix•de>
>> Signed-off-by: Sergey Organov <sorganov@gmail•com>
>> ---
>>  drivers/tty/serial/imx.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 32f36d8..059ba35 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -974,10 +974,22 @@ static void imx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>>  	if (!(port->rs485.flags & SER_RS485_ENABLED)) {
>>  		u32 ucr2;
>>
>> +		/*
>> +		 * Turn off autoRTS if RTS is lowered and restore autoRTS
>> +		 * setting if RTS is raised.
>
> "lower" and "raising" are misleading here. I recommend sticking to
> "active" and "inactive".

This is copy-pasted from the 8250 driver. I'd prefer to leave it as is.

>
>> +		 */
>>  		ucr2 = imx_uart_readl(sport, UCR2);
>>  		ucr2 &= ~(UCR2_CTS | UCR2_CTSC);
>> -		if (mctrl & TIOCM_RTS)
>> -			ucr2 |= UCR2_CTS | UCR2_CTSC;
>> +		if (mctrl & TIOCM_RTS) {
>> +			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.
>> +			 */
>> +			if (!(ucr2 & UCR2_IRTS))
>> +				ucr2 |= UCR2_CTSC;
>> +		}
>
> If you teach imx_uart_rts_auto about IRTS this function could be reused
> here I think.

Yeah, but imx_uart_rts_auto_if_crtscts_and_rts_is_active() ? I feel
somewhat uncomfortable about that mixing of different purposes.

Besides, one of the purposes of these patch series was to get rid of
imx_uart_rts_auto() as its name is confusing in the context of existing
imx_uart_rts_active() and imx_uart_rts_inactive(), as I already
explained before.

We can rename the function to avoid confusion, add yet another check to
it, and call it from 2 places, but it's still questionable if it's an
improvement, and could be done as a follow-up step anyway. It will look
something like this then:

 -- >8 --

    serial: imx: factor out common code into new imx_uart_set_auto_rts()

	Modified   drivers/tty/serial/imx.c

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index d9a73c7..c8b847e 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -954,6 +954,20 @@ static unsigned int imx_uart_get_mctrl(struct uart_port *port)
 	return ret;
 }

+/*
+ * Compute and set auto RTS in 'ucr2' according to the current state of RTS
+ * signal and CRTSCTS state of port configuration.
+ *
+ * Use inverted UCR2_IRTS to get the state of CRTSCTS, and don't let receiver
+ * control RTS output if RTS is inactive.
+ *
+ */
+static void imx_uart_set_auto_rts(u32 *ucr2)
+{
+	if ((*ucr2 & UCR2_CTS) && !(*ucr2 & UCR2_IRTS))
+		*ucr2 |= UCR2_CTSC;
+}
+
 /* called with port.lock taken and irqs off */
 static void imx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
@@ -971,13 +985,7 @@ static void imx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 		ucr2 &= ~(UCR2_CTS | UCR2_CTSC);
 		if (mctrl & TIOCM_RTS) {
 			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.
-			 */
-			if (!(ucr2 & UCR2_IRTS))
-				ucr2 |= UCR2_CTSC;
+			imx_uart_set_auto_rts(&ucr2);
 		}
 		imx_uart_writel(sport, ucr2, UCR2);
 	}
@@ -1594,12 +1602,7 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 			imx_uart_rts_inactive(sport, &ucr2);

 	} else if (termios->c_cflag & CRTSCTS) {
-		/*
-		 * Only let receiver control RTS output if we were not requested
-		 * to have RTS inactive (which then should take precedence).
-		 */
-		if (ucr2 & UCR2_CTS)
-			ucr2 |= UCR2_CTSC;
+		imx_uart_set_auto_rts(&ucr2);
 	}

 	if (termios->c_cflag & CRTSCTS)



-- Sergey

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
