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

Commit Message

Sergey Organov July 22, 2019, 7:22 p.m. UTC

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 <[e-mail]>
Reviewed-by: Sascha Hauer <[e-mail]>
Tested-by: Sascha Hauer <[e-mail]>
Signed-off-by: Sergey Organov <[e-mail]>
---
 drivers/tty/serial/imx.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Uwe Kleine-König July 22, 2019, 8:24 p.m. UTC | #1

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 <[e-mail]>
> Reviewed-by: Sascha Hauer <[e-mail]>
> Tested-by: Sascha Hauer <[e-mail]>
> Signed-off-by: Sergey Organov <[e-mail]>
> ---
>  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".

> +		 */
>  		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.

Best regards
Uwe

Sergey Organov July 23, 2019, 9:20 a.m. UTC | #2

Uwe Kleine-König <[e-mail]> 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 <[e-mail]>
>> Reviewed-by: Sascha Hauer <[e-mail]>
>> Tested-by: Sascha Hauer <[e-mail]>
>> Signed-off-by: Sergey Organov <[e-mail]>
>> ---
>>  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

Uwe Kleine-König July 23, 2019, 9:49 a.m. UTC | #3

On Tue, Jul 23, 2019 at 12:20:38PM +0300, Sergey Organov wrote:
> Uwe Kleine-König <[e-mail]> 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 <[e-mail]>

Oh, you added my Ack for patches 2 and 3, too, even before I looked
again on those :-|

> >> Reviewed-by: Sascha Hauer <[e-mail]>
> >> Tested-by: Sascha Hauer <[e-mail]>
> >> Signed-off-by: Sergey Organov <[e-mail]>
> >> ---
> >>  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.

I'd prefer to fix the 8250 accordingly. "raised" is just misleading
because the handshaking signals are low-active and you always have to
think if the logical or the physical signal is raising. "active" is
clear in this regard.

> >> +		 */
> >>  		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;
> +}
> +

this looks fine and is what I intended to suggest. The comment isn't
optimal yet, I'd write something like:

  /*
   * Enable hardware control of the RTS output iff handshaking is in use
   * and software requested RTS to be active.
   * "handshaking is in use" can be determined from the IRTS bit that is
   * set when handshaking is not used. The requested state by software
   * is represented in the CTS bit.
   */

IMHO go directly to imx_uart_set_auto_rts() before introducing the
second open coding of its logic.

Best regards
Uwe

Sergey Organov July 23, 2019, 11:16 a.m. UTC | #4

Uwe Kleine-König <[e-mail]> writes:

> On Tue, Jul 23, 2019 at 12:20:38PM +0300, Sergey Organov wrote:
>> Uwe Kleine-König <[e-mail]> 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 <[e-mail]>
>
> Oh, you added my Ack for patches 2 and 3, too, even before I looked
> again on those :-|

Oops, I thought you've looked at all 3 before starting to comment.

-- Sergey

Sergey Organov July 23, 2019, 11:30 a.m. UTC | #5

Uwe Kleine-König <[e-mail]> writes:

> On Tue, Jul 23, 2019 at 12:20:38PM +0300, Sergey Organov wrote:
>> Uwe Kleine-König <[e-mail]> 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 <[e-mail]>
>
> Oh, you added my Ack for patches 2 and 3, too, even before I looked
> again on those :-|
>
>> >> Reviewed-by: Sascha Hauer <[e-mail]>
>> >> Tested-by: Sascha Hauer <[e-mail]>
>> >> Signed-off-by: Sergey Organov <[e-mail]>
>> >> ---
>> >>  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.
>
> I'd prefer to fix the 8250 accordingly. "raised" is just misleading
> because the handshaking signals are low-active and you always have to
> think if the logical or the physical signal is raising. "active" is
> clear in this regard.

You are free to do it in one bunch for all the drivers it appears in
then. For now I prefer consistency between drivers, even though I do
agree the wording could be improved.

>> >> +		 */
>> >>  		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;
>> +}
>> +
>
> this looks fine and is what I intended to suggest. The comment isn't
> optimal yet, I'd write something like:
>
>   /*
>    * Enable hardware control of the RTS output iff handshaking is in use
>    * and software requested RTS to be active.
>    * "handshaking is in use" can be determined from the IRTS bit that is
>    * set when handshaking is not used. The requested state by software
>    * is represented in the CTS bit.
>    */

I don't like it as this function doesn't enable or disable anything. It
just computes needed state of one bit taking into account two
conditions.

Independent on comment, for more clarity, it could have been:

static void imx_uart_set_auto_rts(u32 *ucr2)
{
	if ((*ucr2 & UCR2_CTS) && !(*ucr2 & UCR2_IRTS))
		*ucr2 |= UCR2_CTSC;
	else
		*ucr2 &= ~UCR2_CTSC;
}

to be independent of the current state of UCR2_CTSC.

>
> IMHO go directly to imx_uart_set_auto_rts() before introducing the
> second open coding of its logic.

Actually, it looks clearer the way I've suggested, I think, as I didn't
in fact introduce second open coding of its logic, it's just we later
decided that 2 different logics (restore autoRTS on RTS raise, and set
autoRTS on user request) could be merged into single function.

-- Sergey

Patch

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.
+		 */
 		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;
+		}
 		imx_uart_writel(sport, ucr2, UCR2);
 	}