Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> Reviewed-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
>> Tested-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
>> Signed-off-by: Sergey Organov <sorganov@xxxxxxxxx>
>> ---
>> 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
