[PATCH] serial: imx: fix RTS/CTS setting

[PATCH] serial: imx: fix RTS/CTS setting

[Sascha Hauer]

The correct setting of the RTS pin depends on the CRTSCTS termios setting:

- When CRTSCTS is disabled then RTS shall be controlled by the TIOCM_RTS
  flag.
- When CRTSCTS is enabled the expected behaviour of the RTS pin is:
  - When TIOCM_RTS is set then let the receiver control RTS.
  - When the TIOCM_RTS flag is cleared then RTS shall be deasserted (to let
    the upper layers throttle the transfer even when the FIFO in the UART has
    enough space).

This patch fixes this behaviour. Previously the RTS pin has always been
controlled by the receiver once the TIOCM_RTS flag was set and the CRTSCTS
setting hasn't been taken into account.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/tty/serial/imx.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8b752e895053..0eddca6455ad 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -216,6 +216,7 @@ struct imx_port {
 	unsigned int		dma_is_enabled:1;
 	unsigned int		dma_is_rxing:1;
 	unsigned int		dma_is_txing:1;
+	unsigned int		crtscts:1;
 	struct dma_chan		*dma_chan_rx, *dma_chan_tx;
 	struct scatterlist	rx_sgl, tx_sgl[2];
 	void			*rx_buf;
@@ -967,9 +968,18 @@ static void imx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 		u32 ucr2;
 
 		ucr2 = imx_uart_readl(sport, UCR2);
+
 		ucr2 &= ~(UCR2_CTS | UCR2_CTSC);
-		if (mctrl & TIOCM_RTS)
-			ucr2 |= UCR2_CTS | UCR2_CTSC;
+
+		if (mctrl & TIOCM_RTS) {
+			if (sport->crtscts)
+				/* let the receiver control RTS */
+				ucr2 |= UCR2_CTSC;
+			else
+				/* Force RTS active */
+				ucr2 |= UCR2_CTS;
+		}
+
 		imx_uart_writel(sport, ucr2, UCR2);
 	}
 
@@ -1554,6 +1564,11 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	else
 		ucr2 = UCR2_SRST | UCR2_IRTS;
 
+	if (termios->c_cflag & CRTSCTS)
+		sport->crtscts = true;
+	else
+		sport->crtscts = false;
+
 	if (termios->c_cflag & CRTSCTS) {
 		if (sport->have_rtscts) {
 			ucr2 &= ~UCR2_IRTS;
-- 
2.20.1


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

Re: [PATCH] serial: imx: fix RTS/CTS setting

[Uwe Kleine-König]

[expanded Cc: a bit]

Hello Sascha,

On Fri, Jun 14, 2019 at 09:28:01AM +0200, Sascha Hauer wrote:
> The correct setting of the RTS pin depends on the CRTSCTS termios setting:
> 
> - When CRTSCTS is disabled then RTS shall be controlled by the TIOCM_RTS
>   flag.
> - When CRTSCTS is enabled the expected behaviour of the RTS pin is:
>   - When TIOCM_RTS is set then let the receiver control RTS.
>   - When the TIOCM_RTS flag is cleared then RTS shall be deasserted (to let
>     the upper layers throttle the transfer even when the FIFO in the UART has
>     enough space).
> 
> This patch fixes this behaviour. Previously the RTS pin has always been
> controlled by the receiver once the TIOCM_RTS flag was set and the CRTSCTS
> setting hasn't been taken into account.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/tty/serial/imx.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8b752e895053..0eddca6455ad 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -216,6 +216,7 @@ struct imx_port {
>  	unsigned int		dma_is_enabled:1;
>  	unsigned int		dma_is_rxing:1;
>  	unsigned int		dma_is_txing:1;
> +	unsigned int		crtscts:1;
>  	struct dma_chan		*dma_chan_rx, *dma_chan_tx;
>  	struct scatterlist	rx_sgl, tx_sgl[2];
>  	void			*rx_buf;
> @@ -967,9 +968,18 @@ static void imx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  		u32 ucr2;
>  
>  		ucr2 = imx_uart_readl(sport, UCR2);
> +
>  		ucr2 &= ~(UCR2_CTS | UCR2_CTSC);
> -		if (mctrl & TIOCM_RTS)
> -			ucr2 |= UCR2_CTS | UCR2_CTSC;
> +
> +		if (mctrl & TIOCM_RTS) {
> +			if (sport->crtscts)
> +				/* let the receiver control RTS */
> +				ucr2 |= UCR2_CTSC;
> +			else
> +				/* Force RTS active */
> +				ucr2 |= UCR2_CTS;
> +		}
> +

Other drivers check for

	port->status & UPSTAT_AUTORTS

instead of CRTSCTS. I didn't manage to grasp all the details from the
quick look I took, but maybe you should better do the same?

>  		imx_uart_writel(sport, ucr2, UCR2);
>  	}
>  
> @@ -1554,6 +1564,11 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
>  	else
>  		ucr2 = UCR2_SRST | UCR2_IRTS;
>  
> +	if (termios->c_cflag & CRTSCTS)
> +		sport->crtscts = true;
> +	else
> +		sport->crtscts = false;
> +
>  	if (termios->c_cflag & CRTSCTS) {

I'd put setting sport->crtscts in the following if block, maybe even in
the if (sport->have_rtscts) part that starts below here?

>  		if (sport->have_rtscts) {
>  			ucr2 &= ~UCR2_IRTS;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[PATCH RFC 0/7] serial: imx: fix RTS and RTS/CTS handling

[Sergey Organov]

Dear Sascha,

I think these patches deliver simpler end result and are more complete
than what you just posted. In particular, the

  serial: imx: set_mctrl(): correctly restore autoRTS state

addresses exactly the issue your patch is about, but in a slightly
simpler manner.

The patches are not tested yet, so I've put RFC in the header. Just my
2 cents. I can obviously re-roll them on top of your work later, if
required.

Sergey Organov (7):
  serial: imx: fix locking in set_termios()
  serial: imx: set_termios(): factor-out 'ucr2' initial value
  serial: imx: set_termios(): clarify RTS/CTS bits calculation
  serial: imx: set_termios(): do not enable autoRTS if RTS is unset
  serial: imx: set_termios(): preserve RTS state
  serial: imx: set_mctrl(): correctly restore autoRTS state
  serial: imx: get rid of imx_uart_rts_auto()

 drivers/tty/serial/imx.c | 93 ++++++++++++++++++++++++------------------------
 1 file changed, 47 insertions(+), 46 deletions(-)

-- 
2.10.0.1.g57b01a3


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

[PATCH RFC 1/7] serial: imx: fix locking in set_termios()

[Sergey Organov]

imx_uart_set_termios() called imx_uart_rts_active(), or
imx_uart_rts_inactive() before taking port->port.lock.

As a consequence, sport->port.mctrl that these functions modify
could have been changed without holding port->port.lock.

Moved locking of port->port.lock above the calls to fix the issue.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index dff75dc..1055124 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -383,6 +383,7 @@ static void imx_uart_ucrs_restore(struct imx_port *sport,
 }
 #endif
 
+/* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_active(struct imx_port *sport, u32 *ucr2)
 {
 	*ucr2 &= ~(UCR2_CTSC | UCR2_CTS);
@@ -391,6 +392,7 @@ static void imx_uart_rts_active(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
+/* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 {
 	*ucr2 &= ~UCR2_CTSC;
@@ -400,6 +402,7 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
+/* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
 {
 	*ucr2 |= UCR2_CTSC;
@@ -1550,6 +1553,16 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 		old_csize = CS8;
 	}
 
+	del_timer_sync(&sport->timer);
+
+	/*
+	 * Ask the core to calculate the divisor for us.
+	 */
+	baud = uart_get_baud_rate(port, termios, old, 50, port->uartclk / 16);
+	quot = uart_get_divisor(port, baud);
+
+	spin_lock_irqsave(&sport->port.lock, flags);
+
 	if ((termios->c_cflag & CSIZE) == CS8)
 		ucr2 = UCR2_WS | UCR2_SRST | UCR2_IRTS;
 	else
@@ -1593,16 +1606,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 			ucr2 |= UCR2_PROE;
 	}
 
-	del_timer_sync(&sport->timer);
-
-	/*
-	 * Ask the core to calculate the divisor for us.
-	 */
-	baud = uart_get_baud_rate(port, termios, old, 50, port->uartclk / 16);
-	quot = uart_get_divisor(port, baud);
-
-	spin_lock_irqsave(&sport->port.lock, flags);
-
 	sport->port.read_status_mask = 0;
 	if (termios->c_iflag & INPCK)
 		sport->port.read_status_mask |= (URXD_FRMERR | URXD_PRERR);
-- 
2.10.0.1.g57b01a3


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

[PATCH RFC 2/7] serial: imx: set_termios(): factor-out 'ucr2' initial value

[Sergey Organov]

Set common bits in a separate statement to make initialization
explicit and not repeat the common part.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 1055124..87802fd 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1563,10 +1563,9 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 
+	ucr2 = UCR2_SRST | UCR2_IRTS;
 	if ((termios->c_cflag & CSIZE) == CS8)
-		ucr2 = UCR2_WS | UCR2_SRST | UCR2_IRTS;
-	else
-		ucr2 = UCR2_SRST | UCR2_IRTS;
+		ucr2 |= UCR2_WS;
 
 	if (termios->c_cflag & CRTSCTS) {
 		if (sport->have_rtscts) {
-- 
2.10.0.1.g57b01a3


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

[PATCH RFC 3/7] serial: imx: set_termios(): clarify RTS/CTS bits calculation

[Sergey Organov]

Avoid repeating the same code for rs485 twice.

Make it obvious we clear CRTSCTS bit in termios->c_cflag whenever
sport->have_rtscts is false.

Make it obvious we clear UCR2_IRTS whenever CRTSCTS is set.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 87802fd..17e2322 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1567,35 +1567,25 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	if ((termios->c_cflag & CSIZE) == CS8)
 		ucr2 |= UCR2_WS;
 
-	if (termios->c_cflag & CRTSCTS) {
-		if (sport->have_rtscts) {
-			ucr2 &= ~UCR2_IRTS;
+	if (!sport->have_rtscts)
+		termios->c_cflag &= ~CRTSCTS;
 
-			if (port->rs485.flags & SER_RS485_ENABLED) {
-				/*
-				 * RTS is mandatory for rs485 operation, so keep
-				 * it under manual control and keep transmitter
-				 * disabled.
-				 */
-				if (port->rs485.flags &
-				    SER_RS485_RTS_AFTER_SEND)
-					imx_uart_rts_active(sport, &ucr2);
-				else
-					imx_uart_rts_inactive(sport, &ucr2);
-			} else {
-				imx_uart_rts_auto(sport, &ucr2);
-			}
-		} else {
-			termios->c_cflag &= ~CRTSCTS;
-		}
-	} else if (port->rs485.flags & SER_RS485_ENABLED) {
-		/* disable transmitter */
+	if (port->rs485.flags & SER_RS485_ENABLED) {
+		/*
+		 * RTS is mandatory for rs485 operation, so keep
+		 * it under manual control and keep transmitter
+		 * disabled.
+		 */
 		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
 			imx_uart_rts_active(sport, &ucr2);
 		else
 			imx_uart_rts_inactive(sport, &ucr2);
-	}
 
+	} else if (termios->c_cflag & CRTSCTS)
+		imx_uart_rts_auto(sport, &ucr2);
+
+	if (termios->c_cflag & CRTSCTS)
+		ucr2 &= ~UCR2_IRTS;
 
 	if (termios->c_cflag & CSTOPB)
 		ucr2 |= UCR2_STPB;
-- 
2.10.0.1.g57b01a3


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

[PATCH RFC 4/7] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
cleared. Added corresponding check in imx_uart_rts_auto() to fix this.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 17e2322..8ee910f 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 /* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
 {
-	*ucr2 |= UCR2_CTSC;
+	if (*ucr2 & UCR2_CTS)
+		*ucr2 |= UCR2_CTSC;
 }
 
 /* called with port.lock taken and irqs off */
-- 
2.10.0.1.g57b01a3


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

Re: [PATCH RFC 4/7] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sascha Hauer]

Hi Sergey,

On Fri, Jun 14, 2019 at 03:11:31PM +0300, Sergey Organov wrote:
> set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
> cleared. Added corresponding check in imx_uart_rts_auto() to fix this.
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/tty/serial/imx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 17e2322..8ee910f 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>  /* called with port.lock taken and irqs caller dependent */
>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>  {
> -	*ucr2 |= UCR2_CTSC;
> +	if (*ucr2 & UCR2_CTS)
> +		*ucr2 |= UCR2_CTSC;
>  }

*ucr2 is set like this in imx_uart_set_termios():

	ucr2 = UCR2_SRST | UCR2_IRTS;
	if ((termios->c_cflag & CSIZE) == CS8)
		ucr2 |= UCR2_WS;
	...
	imx_uart_rts_auto(sport, &ucr2);

So the UCR2_CTS bit is never set, hence UCR2_CTSC will never be set.
You meant to pass in the actual register value of the UCR2 register.

This is shifted around a bit in the following patches, as an end result
we have:

	old_ucr2 = imx_uart_readl(sport, UCR2);
	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
	...
	if (ucr2 & UCR2_CTS)
		ucr2 |= UCR2_CTSC;

Again the test can never be true, it should probably be if (old_ucr2 &
UCR2_CTS).

With this issue and the one Lothar has found fixed this series works for
me.

With these issues fixed I'd be happy to test this series and apply it in
favour of my patch.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH RFC 4/7] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

Hi Sasha,

Sascha Hauer <s.hauer@pengutronix.de> writes:

> Hi Sergey,
>
> On Fri, Jun 14, 2019 at 03:11:31PM +0300, Sergey Organov wrote:
>> set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
>> cleared. Added corresponding check in imx_uart_rts_auto() to fix this.
>> 
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  drivers/tty/serial/imx.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 17e2322..8ee910f 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>>  /* called with port.lock taken and irqs caller dependent */
>>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>>  {
>> -	*ucr2 |= UCR2_CTSC;
>> +	if (*ucr2 & UCR2_CTS)
>> +		*ucr2 |= UCR2_CTSC;
>>  }
>
> *ucr2 is set like this in imx_uart_set_termios():
>
> 	ucr2 = UCR2_SRST | UCR2_IRTS;
> 	if ((termios->c_cflag & CSIZE) == CS8)
> 		ucr2 |= UCR2_WS;
> 	...
> 	imx_uart_rts_auto(sport, &ucr2);
>
> So the UCR2_CTS bit is never set, hence UCR2_CTSC will never be set.
> You meant to pass in the actual register value of the UCR2 register.
>
> This is shifted around a bit in the following patches, as an end result
> we have:
>
> 	old_ucr2 = imx_uart_readl(sport, UCR2);
> 	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);

This is rather the typo problem in my patches right here: it should have
been:

> 	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);

as we need to preserve RTS bit state USR2_CTS, not hardware handshake bit
UCR2_CCTS.

> 	...
> 	if (ucr2 & UCR2_CTS)
> 		ucr2 |= UCR2_CTSC;
>
> Again the test can never be true, it should probably be if (old_ucr2 &
> UCR2_CTS).

No, I believe it's different mistake on my part, see above.

>
> With this issue and the one Lothar has found fixed this series works for
> me.
>
> With these issues fixed I'd be happy to test this series and apply it in
> favour of my patch.

Thanks a lot for reviewing and volunteering to test! It's even more
appreciated as I can't easily test either on recent kernels and/or
without heavy patching of the kernel, and patching would diminish
applicability of my test results to mainstream kernel.

I think I'll better re-roll the series with these 2 corrections, right?

-- Sergey.

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

[PATCH RFC 5/7] serial: imx: set_termios(): preserve RTS state

[Sergey Organov]

imx_set_termios() cleared RTS on every call, now fixed.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8ee910f..de23068 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1564,6 +1564,13 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 
+	/*
+	 * Read current UCR2 and save it for future use, then clear all the bits
+	 * except those we will or may need to preserve.
+	 */
+	old_ucr2 = imx_uart_readl(sport, UCR2);
+	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
+
 	ucr2 = UCR2_SRST | UCR2_IRTS;
 	if ((termios->c_cflag & CSIZE) == CS8)
 		ucr2 |= UCR2_WS;
@@ -1633,7 +1640,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	imx_uart_writel(sport,
 			old_ucr1 & ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN),
 			UCR1);
-	old_ucr2 = imx_uart_readl(sport, UCR2);
 	imx_uart_writel(sport, old_ucr2 & ~UCR2_ATEN, UCR2);
 
 	while (!(imx_uart_readl(sport, USR2) & USR2_TXDC))
@@ -1641,7 +1647,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	/* then, disable everything */
 	imx_uart_writel(sport, old_ucr2 & ~(UCR2_TXEN | UCR2_RXEN | UCR2_ATEN), UCR2);
-	old_ucr2 &= (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN);
 
 	/* custom-baudrate handling */
 	div = sport->port.uartclk / (baud * 16);
@@ -1679,8 +1684,7 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	imx_uart_writel(sport, old_ucr1, UCR1);
 
-	/* set the parity, stop bits and data size */
-	imx_uart_writel(sport, ucr2 | old_ucr2, UCR2);
+	imx_uart_writel(sport, ucr2, UCR2);
 
 	if (UART_ENABLE_MS(&sport->port, termios->c_cflag))
 		imx_uart_enable_ms(&sport->port);
-- 
2.10.0.1.g57b01a3


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

Re: [PATCH RFC 5/7] serial: imx: set_termios(): preserve RTS state

[Lothar Waßmann]

Hi,

On Fri, 14 Jun 2019 15:11:32 +0300 Sergey Organov wrote:
> imx_set_termios() cleared RTS on every call, now fixed.
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/tty/serial/imx.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8ee910f..de23068 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1564,6 +1564,13 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
>  
>  	spin_lock_irqsave(&sport->port.lock, flags);
>  
> +	/*
> +	 * Read current UCR2 and save it for future use, then clear all the bits
> +	 * except those we will or may need to preserve.
> +	 */
> +	old_ucr2 = imx_uart_readl(sport, UCR2);
> +	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
> +
>  	ucr2 = UCR2_SRST | UCR2_IRTS;
s/=/|=/


Lothar Waßmann

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

Re: [PATCH RFC 5/7] serial: imx: set_termios(): preserve RTS state

[Sergey Organov]

Lothar Waßmann <LW@KARO-electronics.de> writes:
> Hi,
>
> On Fri, 14 Jun 2019 15:11:32 +0300 Sergey Organov wrote:
>> imx_set_termios() cleared RTS on every call, now fixed.
>> 
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  drivers/tty/serial/imx.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 8ee910f..de23068 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -1564,6 +1564,13 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
>>  
>>  	spin_lock_irqsave(&sport->port.lock, flags);
>>  
>> +	/*
>> +	 * Read current UCR2 and save it for future use, then clear all the bits
>> +	 * except those we will or may need to preserve.
>> +	 */
>> +	old_ucr2 = imx_uart_readl(sport, UCR2);
>> +	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
>> +
>>  	ucr2 = UCR2_SRST | UCR2_IRTS;
> s/=/|=/

Nice catch!

Thanks,

-- Sergey Organov

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

[PATCH RFC 6/7] serial: imx: set_mctrl(): correctly restore autoRTS state

[Sergey Organov]

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.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index de23068..bdb8b6a 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -970,10 +970,19 @@ 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 (UCR2_CTSC) if RTS is lowered and restore
+		 * autoRTS setting if RTS is raised. Inverted UCR2_IRTS is set
+		 * if and only if CRTSCTS bit is set for the port, so we use it
+		 * to get the state to restore to.
+		 */
 		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;
+			if (!(ucr2 & UCR2_IRTS))
+				ucr2 |= UCR2_CTSC;
+		}
 		imx_uart_writel(sport, ucr2, UCR2);
 	}
 
-- 
2.10.0.1.g57b01a3


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

[PATCH RFC 7/7] serial: imx: get rid of imx_uart_rts_auto()

[Sergey Organov]

Called in only one place, for RS232, it only obscures things, as it
doesn't go well with 2 similar named functions,
imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
RS485-specific.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index bdb8b6a..cb28cff 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -402,13 +402,6 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
-/* called with port.lock taken and irqs caller dependent */
-static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
-{
-	if (*ucr2 & UCR2_CTS)
-		*ucr2 |= UCR2_CTSC;
-}
-
 /* called with port.lock taken and irqs off */
 static void imx_uart_start_rx(struct uart_port *port)
 {
@@ -1598,8 +1591,10 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 		else
 			imx_uart_rts_inactive(sport, &ucr2);
 
-	} else if (termios->c_cflag & CRTSCTS)
-		imx_uart_rts_auto(sport, &ucr2);
+	} else if (termios->c_cflag & CRTSCTS) {
+		if (ucr2 & UCR2_CTS)
+			ucr2 |= UCR2_CTSC;
+	}
 
 	if (termios->c_cflag & CRTSCTS)
 		ucr2 &= ~UCR2_IRTS;
-- 
2.10.0.1.g57b01a3


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

[PATCH RFC v1 0/7] serial: imx: fix RTS and RTS/CTS handling

[Sergey Organov]

The patches are not tested yet, so the RFC in the header. I'll re-roll
without RFC once Sasha Hauer tests them.

Sasha, in addition to already discussed fixes, I've also reordered 2
patches so that the sequence makes sense.

Changelog:

  v1:

      * Fixed in "serial: imx: set_termios(): preserve RTS state"

-+	ucr2 = UCR2_SRST | UCR2_IRTS;
++	ucr2 |= UCR2_SRST | UCR2_IRTS;
      
        as noticed by Lothar Waßmann <LW@KARO-electronics.de>

      * Fixed in "serial: imx: set_termios(): preserve RTS state"
      
-+	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
++	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);

        as the fix for the problem found by Sascha Hauer
        <s.hauer@pengutronix.de>

      * Reordered:

        serial: imx: set_termios(): preserve RTS state
        serial: imx: set_termios(): do not enable autoRTS if RTS is unset

        as the latter makes sense only provided the former is already applied.
      

Sergey Organov (7):
  serial: imx: fix locking in set_termios()
  serial: imx: set_termios(): factor-out 'ucr2' initial value
  serial: imx: set_termios(): clarify RTS/CTS bits calculation
  serial: imx: set_termios(): preserve RTS state
  serial: imx: set_termios(): do not enable autoRTS if RTS is unset
  serial: imx: set_mctrl(): correctly restore autoRTS state
  serial: imx: get rid of imx_uart_rts_auto()

 drivers/tty/serial/imx.c | 93 ++++++++++++++++++++++++------------------------
 1 file changed, 47 insertions(+), 46 deletions(-)

--
2.10.0.1.g57b01a3

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

[PATCH RFC v1 1/7] serial: imx: fix locking in set_termios()

[Sergey Organov]

imx_uart_set_termios() called imx_uart_rts_active(), or
imx_uart_rts_inactive() before taking port->port.lock.

As a consequence, sport->port.mctrl that these functions modify
could have been changed without holding port->port.lock.

Moved locking of port->port.lock above the calls to fix the issue.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index dff75dc..1055124 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -383,6 +383,7 @@ static void imx_uart_ucrs_restore(struct imx_port *sport,
 }
 #endif
 
+/* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_active(struct imx_port *sport, u32 *ucr2)
 {
 	*ucr2 &= ~(UCR2_CTSC | UCR2_CTS);
@@ -391,6 +392,7 @@ static void imx_uart_rts_active(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
+/* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 {
 	*ucr2 &= ~UCR2_CTSC;
@@ -400,6 +402,7 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
+/* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
 {
 	*ucr2 |= UCR2_CTSC;
@@ -1550,6 +1553,16 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 		old_csize = CS8;
 	}
 
+	del_timer_sync(&sport->timer);
+
+	/*
+	 * Ask the core to calculate the divisor for us.
+	 */
+	baud = uart_get_baud_rate(port, termios, old, 50, port->uartclk / 16);
+	quot = uart_get_divisor(port, baud);
+
+	spin_lock_irqsave(&sport->port.lock, flags);
+
 	if ((termios->c_cflag & CSIZE) == CS8)
 		ucr2 = UCR2_WS | UCR2_SRST | UCR2_IRTS;
 	else
@@ -1593,16 +1606,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 			ucr2 |= UCR2_PROE;
 	}
 
-	del_timer_sync(&sport->timer);
-
-	/*
-	 * Ask the core to calculate the divisor for us.
-	 */
-	baud = uart_get_baud_rate(port, termios, old, 50, port->uartclk / 16);
-	quot = uart_get_divisor(port, baud);
-
-	spin_lock_irqsave(&sport->port.lock, flags);
-
 	sport->port.read_status_mask = 0;
 	if (termios->c_iflag & INPCK)
 		sport->port.read_status_mask |= (URXD_FRMERR | URXD_PRERR);
-- 
2.10.0.1.g57b01a3


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

[PATCH RFC v1 2/7] serial: imx: set_termios(): factor-out 'ucr2' initial value

[Sergey Organov]

Set common bits in a separate statement to make initialization
explicit and not repeat the common part.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 1055124..87802fd 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1563,10 +1563,9 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 
+	ucr2 = UCR2_SRST | UCR2_IRTS;
 	if ((termios->c_cflag & CSIZE) == CS8)
-		ucr2 = UCR2_WS | UCR2_SRST | UCR2_IRTS;
-	else
-		ucr2 = UCR2_SRST | UCR2_IRTS;
+		ucr2 |= UCR2_WS;
 
 	if (termios->c_cflag & CRTSCTS) {
 		if (sport->have_rtscts) {
-- 
2.10.0.1.g57b01a3


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

[PATCH RFC v1 3/7] serial: imx: set_termios(): clarify RTS/CTS bits calculation

[Sergey Organov]

Avoid repeating the same code for rs485 twice.

Make it obvious we clear CRTSCTS bit in termios->c_cflag whenever
sport->have_rtscts is false.

Make it obvious we clear UCR2_IRTS whenever CRTSCTS is set.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 87802fd..17e2322 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1567,35 +1567,25 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	if ((termios->c_cflag & CSIZE) == CS8)
 		ucr2 |= UCR2_WS;
 
-	if (termios->c_cflag & CRTSCTS) {
-		if (sport->have_rtscts) {
-			ucr2 &= ~UCR2_IRTS;
+	if (!sport->have_rtscts)
+		termios->c_cflag &= ~CRTSCTS;
 
-			if (port->rs485.flags & SER_RS485_ENABLED) {
-				/*
-				 * RTS is mandatory for rs485 operation, so keep
-				 * it under manual control and keep transmitter
-				 * disabled.
-				 */
-				if (port->rs485.flags &
-				    SER_RS485_RTS_AFTER_SEND)
-					imx_uart_rts_active(sport, &ucr2);
-				else
-					imx_uart_rts_inactive(sport, &ucr2);
-			} else {
-				imx_uart_rts_auto(sport, &ucr2);
-			}
-		} else {
-			termios->c_cflag &= ~CRTSCTS;
-		}
-	} else if (port->rs485.flags & SER_RS485_ENABLED) {
-		/* disable transmitter */
+	if (port->rs485.flags & SER_RS485_ENABLED) {
+		/*
+		 * RTS is mandatory for rs485 operation, so keep
+		 * it under manual control and keep transmitter
+		 * disabled.
+		 */
 		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
 			imx_uart_rts_active(sport, &ucr2);
 		else
 			imx_uart_rts_inactive(sport, &ucr2);
-	}
 
+	} else if (termios->c_cflag & CRTSCTS)
+		imx_uart_rts_auto(sport, &ucr2);
+
+	if (termios->c_cflag & CRTSCTS)
+		ucr2 &= ~UCR2_IRTS;
 
 	if (termios->c_cflag & CSTOPB)
 		ucr2 |= UCR2_STPB;
-- 
2.10.0.1.g57b01a3


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

[PATCH RFC v1 4/7] serial: imx: set_termios(): preserve RTS state

[Sergey Organov]

imx_set_termios() cleared RTS on every call, now fixed.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 17e2322..e0f5365 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1563,7 +1563,14 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 
-	ucr2 = UCR2_SRST | UCR2_IRTS;
+	/*
+	 * Read current UCR2 and save it for future use, then clear all the bits
+	 * except those we will or may need to preserve.
+	 */
+	old_ucr2 = imx_uart_readl(sport, UCR2);
+	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
+
+	ucr2 |= UCR2_SRST | UCR2_IRTS;
 	if ((termios->c_cflag & CSIZE) == CS8)
 		ucr2 |= UCR2_WS;
 
@@ -1632,7 +1639,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	imx_uart_writel(sport,
 			old_ucr1 & ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN),
 			UCR1);
-	old_ucr2 = imx_uart_readl(sport, UCR2);
 	imx_uart_writel(sport, old_ucr2 & ~UCR2_ATEN, UCR2);
 
 	while (!(imx_uart_readl(sport, USR2) & USR2_TXDC))
@@ -1640,7 +1646,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	/* then, disable everything */
 	imx_uart_writel(sport, old_ucr2 & ~(UCR2_TXEN | UCR2_RXEN | UCR2_ATEN), UCR2);
-	old_ucr2 &= (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN);
 
 	/* custom-baudrate handling */
 	div = sport->port.uartclk / (baud * 16);
@@ -1678,8 +1683,7 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	imx_uart_writel(sport, old_ucr1, UCR1);
 
-	/* set the parity, stop bits and data size */
-	imx_uart_writel(sport, ucr2 | old_ucr2, UCR2);
+	imx_uart_writel(sport, ucr2, UCR2);
 
 	if (UART_ENABLE_MS(&sport->port, termios->c_cflag))
 		imx_uart_enable_ms(&sport->port);
-- 
2.10.0.1.g57b01a3


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

[PATCH RFC v1 5/7] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
cleared. Added corresponding check in imx_uart_rts_auto() to fix this.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e0f5365..4867f80 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 /* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
 {
-	*ucr2 |= UCR2_CTSC;
+	if (*ucr2 & UCR2_CTS)
+		*ucr2 |= UCR2_CTSC;
 }
 
 /* called with port.lock taken and irqs off */
-- 
2.10.0.1.g57b01a3


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

[PATCH RFC v1 6/7] serial: imx: set_mctrl(): correctly restore autoRTS state

[Sergey Organov]

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.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 4867f80..171347d 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -970,10 +970,19 @@ 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 (UCR2_CTSC) if RTS is lowered and restore
+		 * autoRTS setting if RTS is raised. Inverted UCR2_IRTS is set
+		 * if and only if CRTSCTS bit is set for the port, so we use it
+		 * to get the state to restore to.
+		 */
 		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;
+			if (!(ucr2 & UCR2_IRTS))
+				ucr2 |= UCR2_CTSC;
+		}
 		imx_uart_writel(sport, ucr2, UCR2);
 	}
 
-- 
2.10.0.1.g57b01a3


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

[PATCH RFC v1 7/7] serial: imx: get rid of imx_uart_rts_auto()

[Sergey Organov]

Called in only one place, for RS232, it only obscures things, as it
doesn't go well with 2 similar named functions,
imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
RS485-specific.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 171347d..a5e80a0 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -402,13 +402,6 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
-/* called with port.lock taken and irqs caller dependent */
-static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
-{
-	if (*ucr2 & UCR2_CTS)
-		*ucr2 |= UCR2_CTSC;
-}
-
 /* called with port.lock taken and irqs off */
 static void imx_uart_start_rx(struct uart_port *port)
 {
@@ -1598,8 +1591,10 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 		else
 			imx_uart_rts_inactive(sport, &ucr2);
 
-	} else if (termios->c_cflag & CRTSCTS)
-		imx_uart_rts_auto(sport, &ucr2);
+	} else if (termios->c_cflag & CRTSCTS) {
+		if (ucr2 & UCR2_CTS)
+			ucr2 |= UCR2_CTSC;
+	}
 
 	if (termios->c_cflag & CRTSCTS)
 		ucr2 &= ~UCR2_IRTS;
-- 
2.10.0.1.g57b01a3


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

Re: [PATCH RFC v1 0/7] serial: imx: fix RTS and RTS/CTS handling

[Sascha Hauer]

Hi Sergey,

On Thu, Jun 20, 2019 at 05:47:46PM +0300, Sergey Organov wrote:
> The patches are not tested yet, so the RFC in the header. I'll re-roll
> without RFC once Sasha Hauer tests them.
> 
> Sasha, in addition to already discussed fixes, I've also reordered 2
> patches so that the sequence makes sense.
> 

I reviewed and tested this series, so when resending you can add my:

Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
Tested-by: Sascha Hauer <s.hauer@pengutronix.de>

Thanks
 Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH RFC v1 0/7] serial: imx: fix RTS and RTS/CTS handling

[Sergey Organov]

Hi Sasha,

Sascha Hauer <s.hauer@pengutronix.de> writes:
> Hi Sergey,
>
> On Thu, Jun 20, 2019 at 05:47:46PM +0300, Sergey Organov wrote:
>> The patches are not tested yet, so the RFC in the header. I'll re-roll
>> without RFC once Sasha Hauer tests them.
>> 
>> Sasha, in addition to already discussed fixes, I've also reordered 2
>> patches so that the sequence makes sense.
>> 
>
> I reviewed and tested this series, so when resending you can add my:
>
> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> Tested-by: Sascha Hauer <s.hauer@pengutronix.de>

Thank you so much!

Have a nice day,

-- Sergey

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

[PATCH v2 0/7] serial: imx: fix RTS and RTS/CTS handling

[Sergey Organov]

RTS signal and RTS/CTS handshake handling had a few problems these
patches fix.

In addition, minor cleanups are made to the involved code.

Changelog:

  v2:
      * Appended: "Reviewed-by:" and "Tested-by:"
        Sascha Hauer <s.hauer@pengutronix.de>

      * Removed "RFC" from header

  v1:

      * Fixed in "serial: imx: set_termios(): preserve RTS state"

-+	ucr2 = UCR2_SRST | UCR2_IRTS;
++	ucr2 |= UCR2_SRST | UCR2_IRTS;
      
        as noticed by Lothar Waßmann <LW@KARO-electronics.de>

      * Fixed in "serial: imx: set_termios(): preserve RTS state"
      
-+	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
++	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);

        as the fix for the problem found by Sascha Hauer
        <s.hauer@pengutronix.de>

      * Reordered:

        serial: imx: set_termios(): preserve RTS state
        serial: imx: set_termios(): do not enable autoRTS if RTS is unset

        as the latter makes sense only provided the former is already applied.
      

Sergey Organov (7):
  serial: imx: fix locking in set_termios()
  serial: imx: set_termios(): factor-out 'ucr2' initial value
  serial: imx: set_termios(): clarify RTS/CTS bits calculation
  serial: imx: set_termios(): preserve RTS state
  serial: imx: set_termios(): do not enable autoRTS if RTS is unset
  serial: imx: set_mctrl(): correctly restore autoRTS state
  serial: imx: get rid of imx_uart_rts_auto()

 drivers/tty/serial/imx.c | 93 ++++++++++++++++++++++++------------------------
 1 file changed, 47 insertions(+), 46 deletions(-)

--
2.10.0.1.g57b01a3

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

[PATCH v2 1/7] serial: imx: fix locking in set_termios()

[Sergey Organov]

imx_uart_set_termios() called imx_uart_rts_active(), or
imx_uart_rts_inactive() before taking port->port.lock.

As a consequence, sport->port.mctrl that these functions modify
could have been changed without holding port->port.lock.

Moved locking of port->port.lock above the calls to fix the issue.

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 | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index dff75dc..1055124 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -383,6 +383,7 @@ static void imx_uart_ucrs_restore(struct imx_port *sport,
 }
 #endif
 
+/* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_active(struct imx_port *sport, u32 *ucr2)
 {
 	*ucr2 &= ~(UCR2_CTSC | UCR2_CTS);
@@ -391,6 +392,7 @@ static void imx_uart_rts_active(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
+/* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 {
 	*ucr2 &= ~UCR2_CTSC;
@@ -400,6 +402,7 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
+/* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
 {
 	*ucr2 |= UCR2_CTSC;
@@ -1550,6 +1553,16 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 		old_csize = CS8;
 	}
 
+	del_timer_sync(&sport->timer);
+
+	/*
+	 * Ask the core to calculate the divisor for us.
+	 */
+	baud = uart_get_baud_rate(port, termios, old, 50, port->uartclk / 16);
+	quot = uart_get_divisor(port, baud);
+
+	spin_lock_irqsave(&sport->port.lock, flags);
+
 	if ((termios->c_cflag & CSIZE) == CS8)
 		ucr2 = UCR2_WS | UCR2_SRST | UCR2_IRTS;
 	else
@@ -1593,16 +1606,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 			ucr2 |= UCR2_PROE;
 	}
 
-	del_timer_sync(&sport->timer);
-
-	/*
-	 * Ask the core to calculate the divisor for us.
-	 */
-	baud = uart_get_baud_rate(port, termios, old, 50, port->uartclk / 16);
-	quot = uart_get_divisor(port, baud);
-
-	spin_lock_irqsave(&sport->port.lock, flags);
-
 	sport->port.read_status_mask = 0;
 	if (termios->c_iflag & INPCK)
 		sport->port.read_status_mask |= (URXD_FRMERR | URXD_PRERR);
-- 
2.10.0.1.g57b01a3


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

Re: [PATCH v2 1/7] serial: imx: fix locking in set_termios()

[Uwe Kleine-König]

Hello,

On Wed, Jun 26, 2019 at 05:11:27PM +0300, Sergey Organov wrote:
> imx_uart_set_termios() called imx_uart_rts_active(), or
> imx_uart_rts_inactive() before taking port->port.lock.
> 
> As a consequence, sport->port.mctrl that these functions modify
> could have been changed without holding port->port.lock.
> 
> Moved locking of port->port.lock above the calls to fix the issue.
> 
> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[PATCH v2 2/7] serial: imx: set_termios(): factor-out 'ucr2' initial value

[Sergey Organov]

Set common bits in a separate statement to make initialization
explicit and not repeat the common part.

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 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 1055124..87802fd 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1563,10 +1563,9 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 
+	ucr2 = UCR2_SRST | UCR2_IRTS;
 	if ((termios->c_cflag & CSIZE) == CS8)
-		ucr2 = UCR2_WS | UCR2_SRST | UCR2_IRTS;
-	else
-		ucr2 = UCR2_SRST | UCR2_IRTS;
+		ucr2 |= UCR2_WS;
 
 	if (termios->c_cflag & CRTSCTS) {
 		if (sport->have_rtscts) {
-- 
2.10.0.1.g57b01a3


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

Re: [PATCH v2 2/7] serial: imx: set_termios(): factor-out 'ucr2' initial value

[Uwe Kleine-König]

On Wed, Jun 26, 2019 at 05:11:28PM +0300, Sergey Organov wrote:
> Set common bits in a separate statement to make initialization
> explicit and not repeat the common part.
> 
> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>

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

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[PATCH v2 3/7] serial: imx: set_termios(): clarify RTS/CTS bits calculation

[Sergey Organov]

Avoid repeating the same code for rs485 twice.

Make it obvious we clear CRTSCTS bit in termios->c_cflag whenever
sport->have_rtscts is false.

Make it obvious we clear UCR2_IRTS whenever CRTSCTS is set.

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 | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 87802fd..17e2322 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1567,35 +1567,25 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	if ((termios->c_cflag & CSIZE) == CS8)
 		ucr2 |= UCR2_WS;
 
-	if (termios->c_cflag & CRTSCTS) {
-		if (sport->have_rtscts) {
-			ucr2 &= ~UCR2_IRTS;
+	if (!sport->have_rtscts)
+		termios->c_cflag &= ~CRTSCTS;
 
-			if (port->rs485.flags & SER_RS485_ENABLED) {
-				/*
-				 * RTS is mandatory for rs485 operation, so keep
-				 * it under manual control and keep transmitter
-				 * disabled.
-				 */
-				if (port->rs485.flags &
-				    SER_RS485_RTS_AFTER_SEND)
-					imx_uart_rts_active(sport, &ucr2);
-				else
-					imx_uart_rts_inactive(sport, &ucr2);
-			} else {
-				imx_uart_rts_auto(sport, &ucr2);
-			}
-		} else {
-			termios->c_cflag &= ~CRTSCTS;
-		}
-	} else if (port->rs485.flags & SER_RS485_ENABLED) {
-		/* disable transmitter */
+	if (port->rs485.flags & SER_RS485_ENABLED) {
+		/*
+		 * RTS is mandatory for rs485 operation, so keep
+		 * it under manual control and keep transmitter
+		 * disabled.
+		 */
 		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
 			imx_uart_rts_active(sport, &ucr2);
 		else
 			imx_uart_rts_inactive(sport, &ucr2);
-	}
 
+	} else if (termios->c_cflag & CRTSCTS)
+		imx_uart_rts_auto(sport, &ucr2);
+
+	if (termios->c_cflag & CRTSCTS)
+		ucr2 &= ~UCR2_IRTS;
 
 	if (termios->c_cflag & CSTOPB)
 		ucr2 |= UCR2_STPB;
-- 
2.10.0.1.g57b01a3


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

Re: [PATCH v2 3/7] serial: imx: set_termios(): clarify RTS/CTS bits calculation

[Uwe Kleine-König]

On Wed, Jun 26, 2019 at 05:11:29PM +0300, Sergey Organov wrote:
> Avoid repeating the same code for rs485 twice.
> 
> Make it obvious we clear CRTSCTS bit in termios->c_cflag whenever
> sport->have_rtscts is false.
> 
> Make it obvious we clear UCR2_IRTS whenever CRTSCTS is set.
> 
> 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 | 36 +++++++++++++-----------------------
>  1 file changed, 13 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 87802fd..17e2322 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1567,35 +1567,25 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
>  	if ((termios->c_cflag & CSIZE) == CS8)
>  		ucr2 |= UCR2_WS;
>  
> -	if (termios->c_cflag & CRTSCTS) {
> -		if (sport->have_rtscts) {
> -			ucr2 &= ~UCR2_IRTS;
> +	if (!sport->have_rtscts)
> +		termios->c_cflag &= ~CRTSCTS;
>  
> -			if (port->rs485.flags & SER_RS485_ENABLED) {
> -				/*
> -				 * RTS is mandatory for rs485 operation, so keep
> -				 * it under manual control and keep transmitter
> -				 * disabled.
> -				 */
> -				if (port->rs485.flags &
> -				    SER_RS485_RTS_AFTER_SEND)
> -					imx_uart_rts_active(sport, &ucr2);
> -				else
> -					imx_uart_rts_inactive(sport, &ucr2);
> -			} else {
> -				imx_uart_rts_auto(sport, &ucr2);
> -			}
> -		} else {
> -			termios->c_cflag &= ~CRTSCTS;
> -		}
> -	} else if (port->rs485.flags & SER_RS485_ENABLED) {
> -		/* disable transmitter */
> +	if (port->rs485.flags & SER_RS485_ENABLED) {
> +		/*
> +		 * RTS is mandatory for rs485 operation, so keep
> +		 * it under manual control and keep transmitter
> +		 * disabled.
> +		 */
>  		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
>  			imx_uart_rts_active(sport, &ucr2);
>  		else
>  			imx_uart_rts_inactive(sport, &ucr2);
> -	}
>  
> +	} else if (termios->c_cflag & CRTSCTS)
> +		imx_uart_rts_auto(sport, &ucr2);

Here a set of braces is needed even if the body has only a single line.

> +
> +	if (termios->c_cflag & CRTSCTS)
> +		ucr2 &= ~UCR2_IRTS;
>  
>  	if (termios->c_cflag & CSTOPB)
>  		ucr2 |= UCR2_STPB;

Is this patch intended to not change semantic? I wonder if it hides a
fix because if imx_uart_set_termios() was called with termios->c_cflag
& CRTSCTS and !sport->have_rtscts the rs485 block was not reached. Now
it is.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v2 3/7] serial: imx: set_termios(): clarify RTS/CTS bits calculation

[Sergey Organov]

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

> On Wed, Jun 26, 2019 at 05:11:29PM +0300, Sergey Organov wrote:
>> Avoid repeating the same code for rs485 twice.
>> 
>> Make it obvious we clear CRTSCTS bit in termios->c_cflag whenever
>> sport->have_rtscts is false.
>> 
>> Make it obvious we clear UCR2_IRTS whenever CRTSCTS is set.
>> 
>> 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 | 36 +++++++++++++-----------------------
>>  1 file changed, 13 insertions(+), 23 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 87802fd..17e2322 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -1567,35 +1567,25 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
>>  	if ((termios->c_cflag & CSIZE) == CS8)
>>  		ucr2 |= UCR2_WS;
>>  
>> -	if (termios->c_cflag & CRTSCTS) {
>> -		if (sport->have_rtscts) {
>> -			ucr2 &= ~UCR2_IRTS;
>> +	if (!sport->have_rtscts)
>> +		termios->c_cflag &= ~CRTSCTS;
>>  
>> -			if (port->rs485.flags & SER_RS485_ENABLED) {
>> -				/*
>> -				 * RTS is mandatory for rs485 operation, so keep
>> -				 * it under manual control and keep transmitter
>> -				 * disabled.
>> -				 */
>> -				if (port->rs485.flags &
>> -				    SER_RS485_RTS_AFTER_SEND)
>> -					imx_uart_rts_active(sport, &ucr2);
>> -				else
>> -					imx_uart_rts_inactive(sport, &ucr2);
>> -			} else {
>> -				imx_uart_rts_auto(sport, &ucr2);
>> -			}
>> -		} else {
>> -			termios->c_cflag &= ~CRTSCTS;
>> -		}
>> -	} else if (port->rs485.flags & SER_RS485_ENABLED) {
>> -		/* disable transmitter */
>> +	if (port->rs485.flags & SER_RS485_ENABLED) {
>> +		/*
>> +		 * RTS is mandatory for rs485 operation, so keep
>> +		 * it under manual control and keep transmitter
>> +		 * disabled.
>> +		 */
>>  		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
>>  			imx_uart_rts_active(sport, &ucr2);
>>  		else
>>  			imx_uart_rts_inactive(sport, &ucr2);
>> -	}
>>  
>> +	} else if (termios->c_cflag & CRTSCTS)
>> +		imx_uart_rts_auto(sport, &ucr2);
>
> Here a set of braces is needed even if the body has only a single
> line.

Really? scripts/checkpatch.pl didn't catch this.

If needed, is it essential enough to fix here, as final result has this
chunk different anyway (and with braces)?

>
>> +
>> +	if (termios->c_cflag & CRTSCTS)
>> +		ucr2 &= ~UCR2_IRTS;
>>  
>>  	if (termios->c_cflag & CSTOPB)
>>  		ucr2 |= UCR2_STPB;
>
> Is this patch intended to not change semantic? I wonder if it hides a
> fix because if imx_uart_set_termios() was called with termios->c_cflag
> & CRTSCTS and !sport->have_rtscts the rs485 block was not reached. Now
> it is.

As comment says "RTS is mandatory for rs485 operation", I assumed
SER_RS485_ENABLED and !sport->have_rtscts are incompatible, so
there should be no actual semantic change here. I mean:

	if (port->rs485.flags & SER_RS485_ENABLED) {
        	assert(sport->have_rtscts);

should never fire.

Do you think I rather need to put additional check for
sport->have_rtscts inside the SER_RS485_ENABLED case?

Thanks!

-- Sergey

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

[PATCH v2 4/7] serial: imx: set_termios(): preserve RTS state

[Sergey Organov]

imx_set_termios() cleared RTS on every call, now fixed.

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 | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 17e2322..e0f5365 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1563,7 +1563,14 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 
-	ucr2 = UCR2_SRST | UCR2_IRTS;
+	/*
+	 * Read current UCR2 and save it for future use, then clear all the bits
+	 * except those we will or may need to preserve.
+	 */
+	old_ucr2 = imx_uart_readl(sport, UCR2);
+	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
+
+	ucr2 |= UCR2_SRST | UCR2_IRTS;
 	if ((termios->c_cflag & CSIZE) == CS8)
 		ucr2 |= UCR2_WS;
 
@@ -1632,7 +1639,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	imx_uart_writel(sport,
 			old_ucr1 & ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN),
 			UCR1);
-	old_ucr2 = imx_uart_readl(sport, UCR2);
 	imx_uart_writel(sport, old_ucr2 & ~UCR2_ATEN, UCR2);
 
 	while (!(imx_uart_readl(sport, USR2) & USR2_TXDC))
@@ -1640,7 +1646,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	/* then, disable everything */
 	imx_uart_writel(sport, old_ucr2 & ~(UCR2_TXEN | UCR2_RXEN | UCR2_ATEN), UCR2);
-	old_ucr2 &= (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN);
 
 	/* custom-baudrate handling */
 	div = sport->port.uartclk / (baud * 16);
@@ -1678,8 +1683,7 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	imx_uart_writel(sport, old_ucr1, UCR1);
 
-	/* set the parity, stop bits and data size */
-	imx_uart_writel(sport, ucr2 | old_ucr2, UCR2);
+	imx_uart_writel(sport, ucr2, UCR2);
 
 	if (UART_ENABLE_MS(&sport->port, termios->c_cflag))
 		imx_uart_enable_ms(&sport->port);
-- 
2.10.0.1.g57b01a3


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

Re: [PATCH v2 4/7] serial: imx: set_termios(): preserve RTS state

[Uwe Kleine-König]

On Wed, Jun 26, 2019 at 05:11:30PM +0300, Sergey Organov wrote:
> imx_set_termios() cleared RTS on every call, now fixed.

Is this a real problem, or something you noticed by looking at the code?
I think I already asked that in a previous round, if so this should at
least be explained in more detail in the commit log. Also please note
that this is about the UCR2_CTS flag. (It is, isn't it? I don't
understand it after staring at the code for a while.)

Assuming this is a real fix, it would be great if this patch came first
in the series (i.e. before the cleanups) and would be more straight
forward to understand.

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v2 4/7] serial: imx: set_termios(): preserve RTS state

[Sergey Organov]

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

> On Wed, Jun 26, 2019 at 05:11:30PM +0300, Sergey Organov wrote:
>> imx_set_termios() cleared RTS on every call, now fixed.
>
> Is this a real problem, or something you noticed by looking at the code?
> I think I already asked that in a previous round, if so this should at
> least be explained in more detail in the commit log.

Yes, it was real observed problem. Every call to set_termios from user
space (through tcsetattr() function) cleared RTS.

> Also please note that this is about the UCR2_CTS flag. (It is, isn't
> it? I don't understand it after staring at the code for a while.)

"CTS" in iMX terms means what everybody else calls "RTS"! Please notice
how they are used in the entire driver, e.g.:

		if (mctrl & TIOCM_RTS) {
			ucr2 |= UCR2_CTS;


> Assuming this is a real fix, it would be great if this patch came first
> in the series (i.e. before the cleanups) and would be more straight
> forward to understand.

I rather believe that pre-cleanups actually make the fix more
straightforward to understand.

Thanks!

-- Sergey

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

[PATCH v2 5/7] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
cleared. Added corresponding check in imx_uart_rts_auto() to fix this.

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e0f5365..4867f80 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 /* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
 {
-	*ucr2 |= UCR2_CTSC;
+	if (*ucr2 & UCR2_CTS)
+		*ucr2 |= UCR2_CTSC;
 }
 
 /* called with port.lock taken and irqs off */
-- 
2.10.0.1.g57b01a3


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

Re: [PATCH v2 5/7] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Uwe Kleine-König]

On Wed, Jun 26, 2019 at 05:11:31PM +0300, Sergey Organov wrote:
> set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
> cleared. Added corresponding check in imx_uart_rts_auto() to fix this.
> 
> 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index e0f5365..4867f80 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>  /* called with port.lock taken and irqs caller dependent */
>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>  {
> -	*ucr2 |= UCR2_CTSC;
> +	if (*ucr2 & UCR2_CTS)
> +		*ucr2 |= UCR2_CTSC;
>  }

I wonder if this patch is only correct in the presence of the previous
patch. With the code currently in mainline imx_uart_rts_auto() is only
called with UCR2_CTS unset.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v2 5/7] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

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

> On Wed, Jun 26, 2019 at 05:11:31PM +0300, Sergey Organov wrote:
>> set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
>> cleared. Added corresponding check in imx_uart_rts_auto() to fix this.
>> 
>> 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 | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index e0f5365..4867f80 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>>  /* called with port.lock taken and irqs caller dependent */
>>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>>  {
>> -	*ucr2 |= UCR2_CTSC;
>> +	if (*ucr2 & UCR2_CTS)
>> +		*ucr2 |= UCR2_CTSC;
>>  }
>
> I wonder if this patch is only correct in the presence of the previous
> patch. With the code currently in mainline imx_uart_rts_auto() is only
> called with UCR2_CTS unset.

Yes, exactly.

Thanks!

-- Sergey

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

[PATCH v2 6/7] serial: imx: set_mctrl(): correctly restore autoRTS state

[Sergey Organov]

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.

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 | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 4867f80..171347d 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -970,10 +970,19 @@ 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 (UCR2_CTSC) if RTS is lowered and restore
+		 * autoRTS setting if RTS is raised. Inverted UCR2_IRTS is set
+		 * if and only if CRTSCTS bit is set for the port, so we use it
+		 * to get the state to restore to.
+		 */
 		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;
+			if (!(ucr2 & UCR2_IRTS))
+				ucr2 |= UCR2_CTSC;
+		}
 		imx_uart_writel(sport, ucr2, UCR2);
 	}
 
-- 
2.10.0.1.g57b01a3


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

Re: [PATCH v2 6/7] serial: imx: set_mctrl(): correctly restore autoRTS state

[Uwe Kleine-König]

On Wed, Jun 26, 2019 at 05:11:32PM +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.
> 
> 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 | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 4867f80..171347d 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -970,10 +970,19 @@ 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 (UCR2_CTSC) if RTS is lowered and restore
> +		 * autoRTS setting if RTS is raised. Inverted UCR2_IRTS is set
> +		 * if and only if CRTSCTS bit is set for the port, so we use it
> +		 * to get the state to restore to.
> +		 */

The comment is quite complicated. I like the comments of Sascha's patch
that addressed the same issue better.

Are you using UCR2_IRTS as an indicator if CRTSCTS is set? If it's that
what you intend to express in the second sentence that is hard to grasp.
Something like:

	UCR2_IRTS is unset iff the port is configured for CRTSCTS

Also as the value of the CTS bit doesn't matter if CTSC is set, the
order of the checks could be swapped to result in easier code (IMHO at
least) that doesn't need a nested if.

Something like:

	ucr2 = imx_uart_readl(sport, UCR2);
	ucr2 &= ~(UCR2_CTS | UCR2_CTSC);

	/* UCR2_IRTS is unset iff the port is configured for CRTSCTS */
	crtscts = !(ucr2 & UCR2_IRTS);

	if (!(mctrl & TIOCM_RTS)) {
		/* Force RTS inactive, i.e. UCR2_CTS=0 and UCR2_CTSC=0 */
	} else if (crtscts) {
		/* let the receiver control RTS */
		ucr2 |= UCR2_CTSC;
	} else {
		/* Force RTS active */
		ucr2 |= UCR2_CTS;
	}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v2 6/7] serial: imx: set_mctrl(): correctly restore autoRTS state

[Sergey Organov]

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

> On Wed, Jun 26, 2019 at 05:11:32PM +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.
>> 
>> 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 | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 4867f80..171347d 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -970,10 +970,19 @@ 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 (UCR2_CTSC) if RTS is lowered and restore
>> +		 * autoRTS setting if RTS is raised. Inverted UCR2_IRTS is set
>> +		 * if and only if CRTSCTS bit is set for the port, so we use it
>> +		 * to get the state to restore to.
>> +		 */
>
> The comment is quite complicated. I like the comments of Sascha's patch
> that addressed the same issue better.

This one is simply modeled after similar comments in other drivers,
then adding the specifics.

> Are you using UCR2_IRTS as an indicator if CRTSCTS is set? If it's that
> what you intend to express in the second sentence that is hard to grasp.
> Something like:
>
> 	UCR2_IRTS is unset iff the port is configured for CRTSCTS

Yeah, exactly. Fine, I'll change this, thanks!

>
> Also as the value of the CTS bit doesn't matter if CTSC is set, the
> order of the checks could be swapped to result in easier code (IMHO at
> least) that doesn't need a nested if.
>
> Something like:
>
> 	ucr2 = imx_uart_readl(sport, UCR2);
> 	ucr2 &= ~(UCR2_CTS | UCR2_CTSC);
>
> 	/* UCR2_IRTS is unset iff the port is configured for CRTSCTS */
> 	crtscts = !(ucr2 & UCR2_IRTS);
>
> 	if (!(mctrl & TIOCM_RTS)) {
> 		/* Force RTS inactive, i.e. UCR2_CTS=0 and UCR2_CTSC=0 */
> 	} else if (crtscts) {
> 		/* let the receiver control RTS */
> 		ucr2 |= UCR2_CTSC;
> 	} else {
> 		/* Force RTS active */
> 		ucr2 |= UCR2_CTS;
> 	}

Right, this is functionally correct as well, and thus it's a matter of
taste, but I still believe that what I suggested is better:

	ucr2 = imx_uart_readl(sport, UCR2);
	ucr2 &= ~(UCR2_CTS | UCR2_CTSC);
	if (mctrl & TIOCM_RTS) {
		ucr2 |= UCR2_CTS;
		if (!(ucr2 & UCR2_IRTS))
			ucr2 |= UCR2_CTSC;
	}

First, it always sets hardware RTS according to TIOCM_RTS, that IMHO is
less surprising than clearing hardware RTS bit when port is configured
CRTSCTS.

Second, (unfortunate) inter-dependency between TIOCM_RTS and CRTSCTS is
better isolated, so to get rid of it (even if only mentally), only
removals are required, that reduces the code to quite obvious:

	ucr2 = imx_uart_readl(sport, UCR2);
	ucr2 &= ~(UCR2_CTS);
	if (mctrl & TIOCM_RTS)
		ucr2 |= UCR2_CTS;

Thanks!

-- Sergey

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

[PATCH v2 7/7] serial: imx: get rid of imx_uart_rts_auto()

[Sergey Organov]

Called in only one place, for RS232, it only obscures things, as it
doesn't go well with 2 similar named functions,
imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
RS485-specific.

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 | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 171347d..a5e80a0 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -402,13 +402,6 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
-/* called with port.lock taken and irqs caller dependent */
-static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
-{
-	if (*ucr2 & UCR2_CTS)
-		*ucr2 |= UCR2_CTSC;
-}
-
 /* called with port.lock taken and irqs off */
 static void imx_uart_start_rx(struct uart_port *port)
 {
@@ -1598,8 +1591,10 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 		else
 			imx_uart_rts_inactive(sport, &ucr2);
 
-	} else if (termios->c_cflag & CRTSCTS)
-		imx_uart_rts_auto(sport, &ucr2);
+	} else if (termios->c_cflag & CRTSCTS) {
+		if (ucr2 & UCR2_CTS)
+			ucr2 |= UCR2_CTSC;
+	}
 
 	if (termios->c_cflag & CRTSCTS)
 		ucr2 &= ~UCR2_IRTS;
-- 
2.10.0.1.g57b01a3


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

Re: [PATCH v2 7/7] serial: imx: get rid of imx_uart_rts_auto()

[Uwe Kleine-König]

On Wed, Jun 26, 2019 at 05:11:33PM +0300, Sergey Organov wrote:
> Called in only one place, for RS232, it only obscures things, as it
> doesn't go well with 2 similar named functions,
> imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
> RS485-specific.
> 
> 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 | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 171347d..a5e80a0 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -402,13 +402,6 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>  	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
>  }
>  
> -/* called with port.lock taken and irqs caller dependent */
> -static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
> -{
> -	if (*ucr2 & UCR2_CTS)
> -		*ucr2 |= UCR2_CTSC;
> -}
> -
>  /* called with port.lock taken and irqs off */
>  static void imx_uart_start_rx(struct uart_port *port)
>  {
> @@ -1598,8 +1591,10 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
>  		else
>  			imx_uart_rts_inactive(sport, &ucr2);
>  
> -	} else if (termios->c_cflag & CRTSCTS)
> -		imx_uart_rts_auto(sport, &ucr2);
> +	} else if (termios->c_cflag & CRTSCTS) {
> +		if (ucr2 & UCR2_CTS)
> +			ucr2 |= UCR2_CTSC;
> +	}

At least before it was (somewhat) clear that this is about RTS and it
is about something automatic. So I don't like the patch.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v2 7/7] serial: imx: get rid of imx_uart_rts_auto()

[Sergey Organov]

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

> On Wed, Jun 26, 2019 at 05:11:33PM +0300, Sergey Organov wrote:
>> Called in only one place, for RS232, it only obscures things, as it
>> doesn't go well with 2 similar named functions,
>> imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
>> RS485-specific.
>> 
>> 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 | 13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 171347d..a5e80a0 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -402,13 +402,6 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>>  	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
>>  }
>>  
>> -/* called with port.lock taken and irqs caller dependent */
>> -static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>> -{
>> -	if (*ucr2 & UCR2_CTS)
>> -		*ucr2 |= UCR2_CTSC;
>> -}
>> -
>>  /* called with port.lock taken and irqs off */
>>  static void imx_uart_start_rx(struct uart_port *port)
>>  {
>> @@ -1598,8 +1591,10 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
>>  		else
>>  			imx_uart_rts_inactive(sport, &ucr2);
>>  
>> -	} else if (termios->c_cflag & CRTSCTS)
>> -		imx_uart_rts_auto(sport, &ucr2);
>> +	} else if (termios->c_cflag & CRTSCTS) {
>> +		if (ucr2 & UCR2_CTS)
>> +			ucr2 |= UCR2_CTSC;
>> +	}
>
> At least before it was (somewhat) clear that this is about RTS and it
> is about something automatic. So I don't like the patch.

Maybe I just need to put a comment here to clarify?

Let me try to convince you removal is a good thing.

Let's try to mentally revert the patch. If we already have

	} else if (termios->c_cflag & CRTSCTS) {
		if (ucr2 & UCR2_CTS)
			ucr2 |= UCR2_CTSC;
	}

I see no reason to make 2 lines inside if() a function.

First, it's already obvious it's about something automatic, due to if()
condition itself.

Second, the fact that it's about RTS is as [non-]obvious as in any other
place in the driver, taking into account that iMX calls "RTS" "CTS" and
vice versa.

Finally, should we still argue adding a function would be useful, we'd
need to also add, for consistency,

  static void imx_uart_rts_manual(struct imx_port *sport, u32 *ucr2);

(as existing rts_on() and rts_off() do not serve the purpose),

as well as CTS counterparts:

  static void imx_uart_cts_auto(struct imx_port *sport, u32 *ucr2);
  static void imx_uart_cts_manual(struct imx_port *sport, u32 *ucr2);

and patch the code rather heavily, for no obvious gain.

Overall, I believe adding the function would only obscure things.

OTOH, existence of that function forced me to examine the whole source
just to figure that unlike other 2 similar named, it serves entirely
different logical purpose (i.e., it's _not_ 3-d alternative for those
2), and is not used anywhere else.

Look: when we have rts_auto(), rts_off(), and rts_on(), it's logical to
expect it's one of them that will be called when top-level asks for
automatic RTS/CTS, manual RTS off, and manual RTS on, respectively,
isn't it? But it is not the case at all! Still rts_auto() doesn't fit to
the overall picture.

Thanks!

-- Sergey

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

[PATCH v3 0/7] serial: imx: fix RTS and RTS/CTS handling

[Sergey Organov]

RTS signal and RTS/CTS handshake handling had a few problems these
patches fix.

In addition, minor cleanups are made to the involved code.

Changelog:

  v3:

      * Appended: "Reviewed-by:"
        Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
        to the first 2 patches

      * Added braces to one-line 'else if', to the "serial: imx:
        set_termios(): clarify RTS/CTS bits calculation", as suggested
        by Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

      * Improved comments in "serial: imx: set_mctrl(): correctly
        restore autoRTS state", as suggested by Uwe Kleine-König
        <u.kleine-koenig@pengutronix.de>

  v2:

      * Appended: "Reviewed-by:" and "Tested-by:"
        Sascha Hauer <s.hauer@pengutronix.de>

      * Removed "RFC" from header

  v1:

      * Fixed in "serial: imx: set_termios(): preserve RTS state"

-+	ucr2 = UCR2_SRST | UCR2_IRTS;
++	ucr2 |= UCR2_SRST | UCR2_IRTS;

        as noticed by Lothar Waßmann <LW@KARO-electronics.de>

      * Fixed in "serial: imx: set_termios(): preserve RTS state"

-+	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
++	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);

        as the fix for the problem found by Sascha Hauer
        <s.hauer@pengutronix.de>

      * Reordered:

        serial: imx: set_termios(): preserve RTS state
        serial: imx: set_termios(): do not enable autoRTS if RTS is unset

        as the latter makes sense only provided the former is already applied.


Sergey Organov (7):
  serial: imx: fix locking in set_termios()
  serial: imx: set_termios(): factor-out 'ucr2' initial value
  serial: imx: set_termios(): clarify RTS/CTS bits calculation
  serial: imx: set_termios(): preserve RTS state
  serial: imx: set_termios(): do not enable autoRTS if RTS is unset
  serial: imx: set_mctrl(): correctly restore autoRTS state
  serial: imx: get rid of imx_uart_rts_auto()

 drivers/tty/serial/imx.c | 96 +++++++++++++++++++++++++-----------------------
 1 file changed, 50 insertions(+), 46 deletions(-)

--
2.10.0.1.g57b01a3

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

[PATCH v3 1/7] serial: imx: fix locking in set_termios()

[Sergey Organov]

imx_uart_set_termios() called imx_uart_rts_active(), or
imx_uart_rts_inactive() before taking port->port.lock.

As a consequence, sport->port.mctrl that these functions modify
could have been changed without holding port->port.lock.

Moved locking of port->port.lock above the calls to fix the issue.

Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index dff75dc..1055124 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -383,6 +383,7 @@ static void imx_uart_ucrs_restore(struct imx_port *sport,
 }
 #endif
 
+/* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_active(struct imx_port *sport, u32 *ucr2)
 {
 	*ucr2 &= ~(UCR2_CTSC | UCR2_CTS);
@@ -391,6 +392,7 @@ static void imx_uart_rts_active(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
+/* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 {
 	*ucr2 &= ~UCR2_CTSC;
@@ -400,6 +402,7 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
+/* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
 {
 	*ucr2 |= UCR2_CTSC;
@@ -1550,6 +1553,16 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 		old_csize = CS8;
 	}
 
+	del_timer_sync(&sport->timer);
+
+	/*
+	 * Ask the core to calculate the divisor for us.
+	 */
+	baud = uart_get_baud_rate(port, termios, old, 50, port->uartclk / 16);
+	quot = uart_get_divisor(port, baud);
+
+	spin_lock_irqsave(&sport->port.lock, flags);
+
 	if ((termios->c_cflag & CSIZE) == CS8)
 		ucr2 = UCR2_WS | UCR2_SRST | UCR2_IRTS;
 	else
@@ -1593,16 +1606,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 			ucr2 |= UCR2_PROE;
 	}
 
-	del_timer_sync(&sport->timer);
-
-	/*
-	 * Ask the core to calculate the divisor for us.
-	 */
-	baud = uart_get_baud_rate(port, termios, old, 50, port->uartclk / 16);
-	quot = uart_get_divisor(port, baud);
-
-	spin_lock_irqsave(&sport->port.lock, flags);
-
 	sport->port.read_status_mask = 0;
 	if (termios->c_iflag & INPCK)
 		sport->port.read_status_mask |= (URXD_FRMERR | URXD_PRERR);
-- 
2.10.0.1.g57b01a3


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

[PATCH v3 2/7] serial: imx: set_termios(): factor-out 'ucr2' initial value

[Sergey Organov]

Set common bits in a separate statement to make initialization
explicit and not repeat the common part.

Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 1055124..87802fd 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1563,10 +1563,9 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 
+	ucr2 = UCR2_SRST | UCR2_IRTS;
 	if ((termios->c_cflag & CSIZE) == CS8)
-		ucr2 = UCR2_WS | UCR2_SRST | UCR2_IRTS;
-	else
-		ucr2 = UCR2_SRST | UCR2_IRTS;
+		ucr2 |= UCR2_WS;
 
 	if (termios->c_cflag & CRTSCTS) {
 		if (sport->have_rtscts) {
-- 
2.10.0.1.g57b01a3


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

[PATCH v3 3/7] serial: imx: set_termios(): clarify RTS/CTS bits calculation

[Sergey Organov]

Avoid repeating the same code for rs485 twice.

Make it obvious we clear CRTSCTS bit in termios->c_cflag whenever
sport->have_rtscts is false.

Make it obvious we clear UCR2_IRTS whenever CRTSCTS is set.

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 | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 87802fd..17e2322 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1567,35 +1567,25 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	if ((termios->c_cflag & CSIZE) == CS8)
 		ucr2 |= UCR2_WS;
 
-	if (termios->c_cflag & CRTSCTS) {
-		if (sport->have_rtscts) {
-			ucr2 &= ~UCR2_IRTS;
+	if (!sport->have_rtscts)
+		termios->c_cflag &= ~CRTSCTS;
 
-			if (port->rs485.flags & SER_RS485_ENABLED) {
-				/*
-				 * RTS is mandatory for rs485 operation, so keep
-				 * it under manual control and keep transmitter
-				 * disabled.
-				 */
-				if (port->rs485.flags &
-				    SER_RS485_RTS_AFTER_SEND)
-					imx_uart_rts_active(sport, &ucr2);
-				else
-					imx_uart_rts_inactive(sport, &ucr2);
-			} else {
-				imx_uart_rts_auto(sport, &ucr2);
-			}
-		} else {
-			termios->c_cflag &= ~CRTSCTS;
-		}
-	} else if (port->rs485.flags & SER_RS485_ENABLED) {
-		/* disable transmitter */
+	if (port->rs485.flags & SER_RS485_ENABLED) {
+		/*
+		 * RTS is mandatory for rs485 operation, so keep
+		 * it under manual control and keep transmitter
+		 * disabled.
+		 */
 		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
 			imx_uart_rts_active(sport, &ucr2);
 		else
 			imx_uart_rts_inactive(sport, &ucr2);
-	}
 
+	} else if (termios->c_cflag & CRTSCTS)
+		imx_uart_rts_auto(sport, &ucr2);
+
+	if (termios->c_cflag & CRTSCTS)
+		ucr2 &= ~UCR2_IRTS;
 
 	if (termios->c_cflag & CSTOPB)
 		ucr2 |= UCR2_STPB;
-- 
2.10.0.1.g57b01a3


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

[PATCH v3 4/7] serial: imx: set_termios(): preserve RTS state

[Sergey Organov]

imx_set_termios() cleared RTS on every call, now fixed.

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 | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 17e2322..e0f5365 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1563,7 +1563,14 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 
-	ucr2 = UCR2_SRST | UCR2_IRTS;
+	/*
+	 * Read current UCR2 and save it for future use, then clear all the bits
+	 * except those we will or may need to preserve.
+	 */
+	old_ucr2 = imx_uart_readl(sport, UCR2);
+	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
+
+	ucr2 |= UCR2_SRST | UCR2_IRTS;
 	if ((termios->c_cflag & CSIZE) == CS8)
 		ucr2 |= UCR2_WS;
 
@@ -1632,7 +1639,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	imx_uart_writel(sport,
 			old_ucr1 & ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN),
 			UCR1);
-	old_ucr2 = imx_uart_readl(sport, UCR2);
 	imx_uart_writel(sport, old_ucr2 & ~UCR2_ATEN, UCR2);
 
 	while (!(imx_uart_readl(sport, USR2) & USR2_TXDC))
@@ -1640,7 +1646,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	/* then, disable everything */
 	imx_uart_writel(sport, old_ucr2 & ~(UCR2_TXEN | UCR2_RXEN | UCR2_ATEN), UCR2);
-	old_ucr2 &= (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN);
 
 	/* custom-baudrate handling */
 	div = sport->port.uartclk / (baud * 16);
@@ -1678,8 +1683,7 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	imx_uart_writel(sport, old_ucr1, UCR1);
 
-	/* set the parity, stop bits and data size */
-	imx_uart_writel(sport, ucr2 | old_ucr2, UCR2);
+	imx_uart_writel(sport, ucr2, UCR2);
 
 	if (UART_ENABLE_MS(&sport->port, termios->c_cflag))
 		imx_uart_enable_ms(&sport->port);
-- 
2.10.0.1.g57b01a3


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

[PATCH v3 5/7] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
cleared. Added corresponding check in imx_uart_rts_auto() to fix this.

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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e0f5365..5532887 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 /* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
 {
-	*ucr2 |= UCR2_CTSC;
+	if (*ucr2 & UCR2_CTS)
+		*ucr2 |= UCR2_CTSC;
 }
 
 /* called with port.lock taken and irqs off */
@@ -1588,8 +1589,9 @@ 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) {
 		imx_uart_rts_auto(sport, &ucr2);
+	}
 
 	if (termios->c_cflag & CRTSCTS)
 		ucr2 &= ~UCR2_IRTS;
-- 
2.10.0.1.g57b01a3


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

[PATCH v3 6/7] serial: imx: set_mctrl(): correctly restore autoRTS state

[Sergey Organov]

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.

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 5532887..582a3fd 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -970,10 +970,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);
 	}
 
-- 
2.10.0.1.g57b01a3


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

[PATCH v3 7/7] serial: imx: get rid of imx_uart_rts_auto()

[Sergey Organov]

Called in only one place, for RS232, it only obscures things, as it
doesn't go well with 2 similar named functions,
imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
RS485-specific.

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 | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 582a3fd..f4e9d1a 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -402,13 +402,6 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
-/* called with port.lock taken and irqs caller dependent */
-static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
-{
-	if (*ucr2 & UCR2_CTS)
-		*ucr2 |= UCR2_CTSC;
-}
-
 /* called with port.lock taken and irqs off */
 static void imx_uart_start_rx(struct uart_port *port)
 {
@@ -1602,7 +1595,8 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 			imx_uart_rts_inactive(sport, &ucr2);
 
 	} else if (termios->c_cflag & CRTSCTS) {
-		imx_uart_rts_auto(sport, &ucr2);
+		if (ucr2 & UCR2_CTS)
+			ucr2 |= UCR2_CTSC;
 	}
 
 	if (termios->c_cflag & CRTSCTS)
-- 
2.10.0.1.g57b01a3


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

[PATCH v4 0/3] serial: imx: fix RTS and RTS/CTS handling

[Sergey Organov]

This is rebase of v3 on top of 'tty-next', to get rid of commits that
are already adopted to mainline.

RTS signal and RTS/CTS handshake handling had a few problems these
patches fix.

In addition, minor cleanups are made to the involved code.

Changelog:

  v4:

      * rebased on top of 'tty-next', to get rid of commits that
        are already adopted to mainline.

  v3:

      * Improved comments in "serial: imx: set_mctrl(): correctly
        restore autoRTS state", as suggested by Uwe Kleine-König
        <u.kleine-koenig@pengutronix.de>

  v2:

      * Appended: "Reviewed-by:" and "Tested-by:"
        Sascha Hauer <s.hauer@pengutronix.de>

      * Removed "RFC" from header

  v1:

      * Fixed in "serial: imx: set_termios(): preserve RTS state"

-+	ucr2 = UCR2_SRST | UCR2_IRTS;
++	ucr2 |= UCR2_SRST | UCR2_IRTS;

        as noticed by Lothar Waßmann <LW@KARO-electronics.de>

      * Fixed in "serial: imx: set_termios(): preserve RTS state"

-+	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
++	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);

        as the fix for the problem found by Sascha Hauer
        <s.hauer@pengutronix.de>

      * Reordered:

        serial: imx: set_termios(): preserve RTS state
        serial: imx: set_termios(): do not enable autoRTS if RTS is unset

        as the latter makes sense only provided the former is already
        applied.

Sergey Organov (3):
  serial: imx: set_termios(): do not enable autoRTS if RTS is unset
  serial: imx: set_mctrl(): correctly restore autoRTS state
  serial: imx: get rid of imx_uart_rts_auto()

 drivers/tty/serial/imx.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

--
2.10.0.1.g57b01a3


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

[PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
cleared. Added corresponding check in imx_uart_rts_auto() to fix this.

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 57d6e6b..95d7984 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 /* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
 {
-	*ucr2 |= UCR2_CTSC;
+	if (*ucr2 & UCR2_CTS)
+		*ucr2 |= UCR2_CTSC;
 }
 
 /* called with port.lock taken and irqs off */
-- 
2.10.0.1.g57b01a3


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

Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Uwe Kleine-König]

On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
> set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
> cleared. Added corresponding check in imx_uart_rts_auto() to fix this.

This is not understandable unless you read the reference manual.

In terms understandable without manual, this patch does:

	Don't let the receiver hardware control the RTS output if it was
	requested to be inactive.

> 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 57d6e6b..95d7984 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>  /* called with port.lock taken and irqs caller dependent */
>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>  {
> -	*ucr2 |= UCR2_CTSC;
> +	if (*ucr2 & UCR2_CTS)
> +		*ucr2 |= UCR2_CTSC;

I think this patch is wrong or the commit log is insufficient.
imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
never true. And CTSC isn't restored anywhere, is it?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

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

> On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
>> set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
>> cleared. Added corresponding check in imx_uart_rts_auto() to fix this.
>
> This is not understandable unless you read the reference manual.
>
> In terms understandable without manual, this patch does:
>
> 	Don't let the receiver hardware control the RTS output if it was
> 	requested to be inactive.

I'll fix it, thanks!

>
>> 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 | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 57d6e6b..95d7984 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>>  /* called with port.lock taken and irqs caller dependent */
>>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>>  {
>> -	*ucr2 |= UCR2_CTSC;
>> +	if (*ucr2 & UCR2_CTS)
>> +		*ucr2 |= UCR2_CTSC;
>
> I think this patch is wrong or the commit log is insufficient.
> imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
> never true. And CTSC isn't restored anywhere, is it?

This is rebase to 'tty-next' branch, and you need to look at it in that
context. There, ucr2 & UCR2_CTS does already make sense, due to previous
fix that is already there.

Alternatively, look at v3 of the patches, but you basically already did.
It's that the first part of v3 patch series has been already accepted
upstream, and this is the rest of those patches.

Thanks!

-- Sergey

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

Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Uwe Kleine-König]

Hello Sergey,

On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >> index 57d6e6b..95d7984 100644
> >> --- a/drivers/tty/serial/imx.c
> >> +++ b/drivers/tty/serial/imx.c
> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
> >>  /* called with port.lock taken and irqs caller dependent */
> >>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
> >>  {
> >> -	*ucr2 |= UCR2_CTSC;
> >> +	if (*ucr2 & UCR2_CTS)
> >> +		*ucr2 |= UCR2_CTSC;
> >
> > I think this patch is wrong or the commit log is insufficient.
> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
> > never true. And CTSC isn't restored anywhere, is it?
> 
> This is rebase to 'tty-next' branch, and you need to look at it in that
> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
> fix that is already there.

I looked at 57d6e6b which is the file you patched. And there
imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.

If you still think I'm wrong, please improve the commit log accordingly.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

Hello Uwe,

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> Hello Sergey,
>
> On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
>> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> >> index 57d6e6b..95d7984 100644
>> >> --- a/drivers/tty/serial/imx.c
>> >> +++ b/drivers/tty/serial/imx.c
>> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>> >>  /* called with port.lock taken and irqs caller dependent */
>> >>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>> >>  {
>> >> -	*ucr2 |= UCR2_CTSC;
>> >> +	if (*ucr2 & UCR2_CTS)
>> >> +		*ucr2 |= UCR2_CTSC;
>> >
>> > I think this patch is wrong or the commit log is insufficient.
>> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
>> > never true. And CTSC isn't restored anywhere, is it?
>> 
>> This is rebase to 'tty-next' branch, and you need to look at it in that
>> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
>> fix that is already there.
>
> I looked at 57d6e6b which is the file you patched. And there
> imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
>
> If you still think I'm wrong, please improve the commit log
> accordingly.

I still think you are wrong, but I don't know how to improve commit log.

To check it once again, I just did:

$ git show 57d6e6b > imx.c

There, in imx_uart_set_termios(), I see:

1569:	old_ucr2 = imx_uart_readl(sport, UCR2);
1570:	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);

Here, current UCR2 value is read into 'old_ucr2' and then its /current/
UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).

Then, later in the same function:

1591:		imx_uart_rts_auto(sport, &ucr2);

is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.

That's what the patch does, checks this bit.

Sorry, I fail to see how you came to conclusion that "*ucr2 not having
UCR2_CTS". Do we maybe still read different versions of the file?

-- Sergey

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

Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Uwe Kleine-König]

On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
> Hello Uwe,
> 
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > Hello Sergey,
> >
> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >> >> index 57d6e6b..95d7984 100644
> >> >> --- a/drivers/tty/serial/imx.c
> >> >> +++ b/drivers/tty/serial/imx.c
> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
> >> >>  /* called with port.lock taken and irqs caller dependent */
> >> >>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
> >> >>  {
> >> >> -	*ucr2 |= UCR2_CTSC;
> >> >> +	if (*ucr2 & UCR2_CTS)
> >> >> +		*ucr2 |= UCR2_CTSC;
> >> >
> >> > I think this patch is wrong or the commit log is insufficient.
> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
> >> > never true. And CTSC isn't restored anywhere, is it?
> >> 
> >> This is rebase to 'tty-next' branch, and you need to look at it in that
> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
> >> fix that is already there.
> >
> > I looked at 57d6e6b which is the file you patched. And there
> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
> >
> > If you still think I'm wrong, please improve the commit log
> > accordingly.
> 
> I still think you are wrong, but I don't know how to improve commit log.
> 
> To check it once again, I just did:
> 
> $ git show 57d6e6b > imx.c
> 
> There, in imx_uart_set_termios(), I see:
> 
> 1569:	old_ucr2 = imx_uart_readl(sport, UCR2);
> 1570:	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
> 
> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
> 
> Then, later in the same function:
> 
> 1591:		imx_uart_rts_auto(sport, &ucr2);
> 
> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
> 
> That's what the patch does, checks this bit.
> 
> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
> UCR2_CTS". Do we maybe still read different versions of the file?

No, it's just that I failed to see that UCR2_CTS is in the set of bits
that are retained even when looking twice :-|

So you convinced me that you are right and if you update the commit log
as agreed already before and even add a comment in imx_uart_rts_auto
along the lines of

	/*
	 * Only let the receiver control RTS output if we were not
	 * requested to have RTS inactive (which then should take
	 * precedence).
	 */

you can have my Ack.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

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

> On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
>> Hello Uwe,
>> 
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> > Hello Sergey,
>> >
>> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
>> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
>> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> >> >> index 57d6e6b..95d7984 100644
>> >> >> --- a/drivers/tty/serial/imx.c
>> >> >> +++ b/drivers/tty/serial/imx.c
>> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>> >> >>  /* called with port.lock taken and irqs caller dependent */
>> >> >>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>> >> >>  {
>> >> >> -	*ucr2 |= UCR2_CTSC;
>> >> >> +	if (*ucr2 & UCR2_CTS)
>> >> >> +		*ucr2 |= UCR2_CTSC;
>> >> >
>> >> > I think this patch is wrong or the commit log is insufficient.
>> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
>> >> > never true. And CTSC isn't restored anywhere, is it?
>> >> 
>> >> This is rebase to 'tty-next' branch, and you need to look at it in that
>> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
>> >> fix that is already there.
>> >
>> > I looked at 57d6e6b which is the file you patched. And there
>> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
>> >
>> > If you still think I'm wrong, please improve the commit log
>> > accordingly.
>> 
>> I still think you are wrong, but I don't know how to improve commit log.
>> 
>> To check it once again, I just did:
>> 
>> $ git show 57d6e6b > imx.c
>> 
>> There, in imx_uart_set_termios(), I see:
>> 
>> 1569:	old_ucr2 = imx_uart_readl(sport, UCR2);
>> 1570:	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
>> 
>> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
>> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
>> 
>> Then, later in the same function:
>> 
>> 1591:		imx_uart_rts_auto(sport, &ucr2);
>> 
>> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
>> 
>> That's what the patch does, checks this bit.
>> 
>> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
>> UCR2_CTS". Do we maybe still read different versions of the file?
>
> No, it's just that I failed to see that UCR2_CTS is in the set of bits
> that are retained even when looking twice :-|

Ah, that one... How familiar :-)

> So you convinced me that you are right and if you update the commit log
> as agreed already before and even add a comment in imx_uart_rts_auto
> along the lines of
>
> 	/*
> 	 * Only let the receiver control RTS output if we were not
> 	 * requested to have RTS inactive (which then should take
> 	 * precedence).
> 	 */
>
> you can have my Ack.

OK, will do

Thanks!

-- Sergey

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

Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Uwe Kleine-König]

On Mon, Jul 22, 2019 at 10:42:57AM +0300, Sergey Organov wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> 
> > On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
> >> Hello Uwe,
> >> 
> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> > Hello Sergey,
> >> >
> >> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
> >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
> >> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >> >> >> index 57d6e6b..95d7984 100644
> >> >> >> --- a/drivers/tty/serial/imx.c
> >> >> >> +++ b/drivers/tty/serial/imx.c
> >> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
> >> >> >>  /* called with port.lock taken and irqs caller dependent */
> >> >> >>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
> >> >> >>  {
> >> >> >> -	*ucr2 |= UCR2_CTSC;
> >> >> >> +	if (*ucr2 & UCR2_CTS)
> >> >> >> +		*ucr2 |= UCR2_CTSC;
> >> >> >
> >> >> > I think this patch is wrong or the commit log is insufficient.
> >> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
> >> >> > never true. And CTSC isn't restored anywhere, is it?
> >> >> 
> >> >> This is rebase to 'tty-next' branch, and you need to look at it in that
> >> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
> >> >> fix that is already there.
> >> >
> >> > I looked at 57d6e6b which is the file you patched. And there
> >> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
> >> >
> >> > If you still think I'm wrong, please improve the commit log
> >> > accordingly.
> >> 
> >> I still think you are wrong, but I don't know how to improve commit log.
> >> 
> >> To check it once again, I just did:
> >> 
> >> $ git show 57d6e6b > imx.c
> >> 
> >> There, in imx_uart_set_termios(), I see:
> >> 
> >> 1569:	old_ucr2 = imx_uart_readl(sport, UCR2);
> >> 1570:	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
> >> 
> >> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
> >> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
> >> 
> >> Then, later in the same function:
> >> 
> >> 1591:		imx_uart_rts_auto(sport, &ucr2);
> >> 
> >> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
> >> 
> >> That's what the patch does, checks this bit.
> >> 
> >> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
> >> UCR2_CTS". Do we maybe still read different versions of the file?
> >
> > No, it's just that I failed to see that UCR2_CTS is in the set of bits
> > that are retained even when looking twice :-|
> 
> Ah, that one... How familiar :-)

I thought again a bit over the weekend about this. I wonder if it's
correct to keep RTS active while going through .set_termios. Shouldn't
it maybe always be inactive to prevent the other side sending data while
we are changing the baud rate?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

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

> On Mon, Jul 22, 2019 at 10:42:57AM +0300, Sergey Organov wrote:
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> 
>> > On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
>> >> Hello Uwe,
>> >> 
>> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> >> > Hello Sergey,
>> >> >
>> >> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
>> >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> >> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
>> >> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> >> >> >> index 57d6e6b..95d7984 100644
>> >> >> >> --- a/drivers/tty/serial/imx.c
>> >> >> >> +++ b/drivers/tty/serial/imx.c
>> >> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>> >> >> >>  /* called with port.lock taken and irqs caller dependent */
>> >> >> >>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>> >> >> >>  {
>> >> >> >> -	*ucr2 |= UCR2_CTSC;
>> >> >> >> +	if (*ucr2 & UCR2_CTS)
>> >> >> >> +		*ucr2 |= UCR2_CTSC;
>> >> >> >
>> >> >> > I think this patch is wrong or the commit log is insufficient.
>> >> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
>> >> >> > never true. And CTSC isn't restored anywhere, is it?
>> >> >> 
>> >> >> This is rebase to 'tty-next' branch, and you need to look at it in that
>> >> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
>> >> >> fix that is already there.
>> >> >
>> >> > I looked at 57d6e6b which is the file you patched. And there
>> >> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
>> >> >
>> >> > If you still think I'm wrong, please improve the commit log
>> >> > accordingly.
>> >> 
>> >> I still think you are wrong, but I don't know how to improve commit log.
>> >> 
>> >> To check it once again, I just did:
>> >> 
>> >> $ git show 57d6e6b > imx.c
>> >> 
>> >> There, in imx_uart_set_termios(), I see:
>> >> 
>> >> 1569:	old_ucr2 = imx_uart_readl(sport, UCR2);
>> >> 1570:	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
>> >> 
>> >> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
>> >> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
>> >> 
>> >> Then, later in the same function:
>> >> 
>> >> 1591:		imx_uart_rts_auto(sport, &ucr2);
>> >> 
>> >> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
>> >> 
>> >> That's what the patch does, checks this bit.
>> >> 
>> >> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
>> >> UCR2_CTS". Do we maybe still read different versions of the file?
>> >
>> > No, it's just that I failed to see that UCR2_CTS is in the set of bits
>> > that are retained even when looking twice :-|
>> 
>> Ah, that one... How familiar :-)
>
> I thought again a bit over the weekend about this. I wonder if it's
> correct to keep RTS active while going through .set_termios. Shouldn't
> it maybe always be inactive to prevent the other side sending data while
> we are changing the baud rate?

I don't think it's a good idea to change RTS state over .set_terimios,
as it doesn't in fact solve anything (notice that the other end should
also change baud rate accordingly), and changes visible state (even if
temporarily) that it was not asked to change, that could in turn lead to
utter surprises.

Correct changing of baud rates, bits, etc., could only be implemented at
communication protocol level (read: application level), and there are
all the tools in the kernel to do it right, provided driver does not do
what it was not asked to do.

-- Sergey

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

Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Uwe Kleine-König]

On Mon, Jul 22, 2019 at 12:20:02PM +0300, Sergey Organov wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> 
> > On Mon, Jul 22, 2019 at 10:42:57AM +0300, Sergey Organov wrote:
> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> 
> >> > On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
> >> >> Hello Uwe,
> >> >> 
> >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> >> > Hello Sergey,
> >> >> >
> >> >> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
> >> >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> >> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
> >> >> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >> >> >> >> index 57d6e6b..95d7984 100644
> >> >> >> >> --- a/drivers/tty/serial/imx.c
> >> >> >> >> +++ b/drivers/tty/serial/imx.c
> >> >> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
> >> >> >> >>  /* called with port.lock taken and irqs caller dependent */
> >> >> >> >>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
> >> >> >> >>  {
> >> >> >> >> -	*ucr2 |= UCR2_CTSC;
> >> >> >> >> +	if (*ucr2 & UCR2_CTS)
> >> >> >> >> +		*ucr2 |= UCR2_CTSC;
> >> >> >> >
> >> >> >> > I think this patch is wrong or the commit log is insufficient.
> >> >> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
> >> >> >> > never true. And CTSC isn't restored anywhere, is it?
> >> >> >> 
> >> >> >> This is rebase to 'tty-next' branch, and you need to look at it in that
> >> >> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
> >> >> >> fix that is already there.
> >> >> >
> >> >> > I looked at 57d6e6b which is the file you patched. And there
> >> >> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
> >> >> >
> >> >> > If you still think I'm wrong, please improve the commit log
> >> >> > accordingly.
> >> >> 
> >> >> I still think you are wrong, but I don't know how to improve commit log.
> >> >> 
> >> >> To check it once again, I just did:
> >> >> 
> >> >> $ git show 57d6e6b > imx.c
> >> >> 
> >> >> There, in imx_uart_set_termios(), I see:
> >> >> 
> >> >> 1569:	old_ucr2 = imx_uart_readl(sport, UCR2);
> >> >> 1570:	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
> >> >> 
> >> >> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
> >> >> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
> >> >> 
> >> >> Then, later in the same function:
> >> >> 
> >> >> 1591:		imx_uart_rts_auto(sport, &ucr2);
> >> >> 
> >> >> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
> >> >> 
> >> >> That's what the patch does, checks this bit.
> >> >> 
> >> >> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
> >> >> UCR2_CTS". Do we maybe still read different versions of the file?
> >> >
> >> > No, it's just that I failed to see that UCR2_CTS is in the set of bits
> >> > that are retained even when looking twice :-|
> >> 
> >> Ah, that one... How familiar :-)
> >
> > I thought again a bit over the weekend about this. I wonder if it's
> > correct to keep RTS active while going through .set_termios. Shouldn't
> > it maybe always be inactive to prevent the other side sending data while
> > we are changing the baud rate?
> 
> I don't think it's a good idea to change RTS state over .set_terimios,
> as it doesn't in fact solve anything (notice that the other end should
> also change baud rate accordingly), and changes visible state (even if
> temporarily) that it was not asked to change, that could in turn lead to
> utter surprises.

It should for sure not be done in imx-uart specific code. But I think
that deasserting RTS before calling .set_termios (iff rtscts is enabled)
is a sane thing to do for generic code. I don't want to motivate the
other side to send data while I reconfigure my receiver. Yes, this is a
visible change, but IMHO a good one.

> Correct changing of baud rates, bits, etc., could only be implemented at
> communication protocol level (read: application level), and there are
> all the tools in the kernel to do it right, provided driver does not do
> what it was not asked to do.

Hmm, deasserting RTS while not being ready helps here. Otherwise the
communication partner that sends first after both agreed to change the
baud rate might start doing that before the receiver on the other end is
done. When RTS is deasserted this race window is at least smaller.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

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

> On Mon, Jul 22, 2019 at 12:20:02PM +0300, Sergey Organov wrote:
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> 
>> > On Mon, Jul 22, 2019 at 10:42:57AM +0300, Sergey Organov wrote:
>> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> >> 
>> >> > On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
>> >> >> Hello Uwe,
>> >> >> 
>> >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> >> >> > Hello Sergey,
>> >> >> >
>> >> >> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
>> >> >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> >> >> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
>> >> >> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> >> >> >> >> index 57d6e6b..95d7984 100644
>> >> >> >> >> --- a/drivers/tty/serial/imx.c
>> >> >> >> >> +++ b/drivers/tty/serial/imx.c
>> >> >> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>> >> >> >> >>  /* called with port.lock taken and irqs caller dependent */
>> >> >> >> >>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>> >> >> >> >>  {
>> >> >> >> >> -	*ucr2 |= UCR2_CTSC;
>> >> >> >> >> +	if (*ucr2 & UCR2_CTS)
>> >> >> >> >> +		*ucr2 |= UCR2_CTSC;
>> >> >> >> >
>> >> >> >> > I think this patch is wrong or the commit log is insufficient.
>> >> >> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
>> >> >> >> > never true. And CTSC isn't restored anywhere, is it?
>> >> >> >> 
>> >> >> >> This is rebase to 'tty-next' branch, and you need to look at it in that
>> >> >> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
>> >> >> >> fix that is already there.
>> >> >> >
>> >> >> > I looked at 57d6e6b which is the file you patched. And there
>> >> >> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
>> >> >> >
>> >> >> > If you still think I'm wrong, please improve the commit log
>> >> >> > accordingly.
>> >> >> 
>> >> >> I still think you are wrong, but I don't know how to improve commit log.
>> >> >> 
>> >> >> To check it once again, I just did:
>> >> >> 
>> >> >> $ git show 57d6e6b > imx.c
>> >> >> 
>> >> >> There, in imx_uart_set_termios(), I see:
>> >> >> 
>> >> >> 1569:	old_ucr2 = imx_uart_readl(sport, UCR2);
>> >> >> 1570:	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
>> >> >> 
>> >> >> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
>> >> >> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
>> >> >> 
>> >> >> Then, later in the same function:
>> >> >> 
>> >> >> 1591:		imx_uart_rts_auto(sport, &ucr2);
>> >> >> 
>> >> >> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
>> >> >> 
>> >> >> That's what the patch does, checks this bit.
>> >> >> 
>> >> >> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
>> >> >> UCR2_CTS". Do we maybe still read different versions of the file?
>> >> >
>> >> > No, it's just that I failed to see that UCR2_CTS is in the set of bits
>> >> > that are retained even when looking twice :-|
>> >> 
>> >> Ah, that one... How familiar :-)
>> >
>> > I thought again a bit over the weekend about this. I wonder if it's
>> > correct to keep RTS active while going through .set_termios. Shouldn't
>> > it maybe always be inactive to prevent the other side sending data while
>> > we are changing the baud rate?
>> 
>> I don't think it's a good idea to change RTS state over .set_terimios,
>> as it doesn't in fact solve anything (notice that the other end should
>> also change baud rate accordingly), and changes visible state (even if
>> temporarily) that it was not asked to change, that could in turn lead to
>> utter surprises.
>
> It should for sure not be done in imx-uart specific code. But I think
> that deasserting RTS before calling .set_termios (iff rtscts is enabled)
> is a sane thing to do for generic code. I don't want to motivate the
> other side to send data while I reconfigure my receiver. Yes, this is a
> visible change, but IMHO a good one.
>
>> Correct changing of baud rates, bits, etc., could only be implemented at
>> communication protocol level (read: application level), and there are
>> all the tools in the kernel to do it right, provided driver does not do
>> what it was not asked to do.
>
> Hmm, deasserting RTS while not being ready helps here. Otherwise the
> communication partner that sends first after both agreed to change the
> baud rate might start doing that before the receiver on the other end is
> done. When RTS is deasserted this race window is at least smaller.

In general, it's a wrong idea to do in the kernel what could be done as
efficiently at application level.

In this particular case, application is free to deassert RTS before it
decides to change communication parameters, if it actually needs to, and
thus kernel is wrong level for doing this.

Best Regards,

-- Sergey

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

Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Uwe Kleine-König]

On Mon, Jul 22, 2019 at 04:54:28PM +0300, Sergey Organov wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> 
> > On Mon, Jul 22, 2019 at 12:20:02PM +0300, Sergey Organov wrote:
> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> 
> >> > On Mon, Jul 22, 2019 at 10:42:57AM +0300, Sergey Organov wrote:
> >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> >> 
> >> >> > On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
> >> >> >> Hello Uwe,
> >> >> >> 
> >> >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> >> >> > Hello Sergey,
> >> >> >> >
> >> >> >> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
> >> >> >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> >> >> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
> >> >> >> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >> >> >> >> >> index 57d6e6b..95d7984 100644
> >> >> >> >> >> --- a/drivers/tty/serial/imx.c
> >> >> >> >> >> +++ b/drivers/tty/serial/imx.c
> >> >> >> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
> >> >> >> >> >>  /* called with port.lock taken and irqs caller dependent */
> >> >> >> >> >>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
> >> >> >> >> >>  {
> >> >> >> >> >> -	*ucr2 |= UCR2_CTSC;
> >> >> >> >> >> +	if (*ucr2 & UCR2_CTS)
> >> >> >> >> >> +		*ucr2 |= UCR2_CTSC;
> >> >> >> >> >
> >> >> >> >> > I think this patch is wrong or the commit log is insufficient.
> >> >> >> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
> >> >> >> >> > never true. And CTSC isn't restored anywhere, is it?
> >> >> >> >> 
> >> >> >> >> This is rebase to 'tty-next' branch, and you need to look at it in that
> >> >> >> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
> >> >> >> >> fix that is already there.
> >> >> >> >
> >> >> >> > I looked at 57d6e6b which is the file you patched. And there
> >> >> >> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
> >> >> >> >
> >> >> >> > If you still think I'm wrong, please improve the commit log
> >> >> >> > accordingly.
> >> >> >> 
> >> >> >> I still think you are wrong, but I don't know how to improve commit log.
> >> >> >> 
> >> >> >> To check it once again, I just did:
> >> >> >> 
> >> >> >> $ git show 57d6e6b > imx.c
> >> >> >> 
> >> >> >> There, in imx_uart_set_termios(), I see:
> >> >> >> 
> >> >> >> 1569:	old_ucr2 = imx_uart_readl(sport, UCR2);
> >> >> >> 1570:	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
> >> >> >> 
> >> >> >> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
> >> >> >> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
> >> >> >> 
> >> >> >> Then, later in the same function:
> >> >> >> 
> >> >> >> 1591:		imx_uart_rts_auto(sport, &ucr2);
> >> >> >> 
> >> >> >> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
> >> >> >> 
> >> >> >> That's what the patch does, checks this bit.
> >> >> >> 
> >> >> >> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
> >> >> >> UCR2_CTS". Do we maybe still read different versions of the file?
> >> >> >
> >> >> > No, it's just that I failed to see that UCR2_CTS is in the set of bits
> >> >> > that are retained even when looking twice :-|
> >> >> 
> >> >> Ah, that one... How familiar :-)
> >> >
> >> > I thought again a bit over the weekend about this. I wonder if it's
> >> > correct to keep RTS active while going through .set_termios. Shouldn't
> >> > it maybe always be inactive to prevent the other side sending data while
> >> > we are changing the baud rate?
> >> 
> >> I don't think it's a good idea to change RTS state over .set_terimios,
> >> as it doesn't in fact solve anything (notice that the other end should
> >> also change baud rate accordingly), and changes visible state (even if
> >> temporarily) that it was not asked to change, that could in turn lead to
> >> utter surprises.
> >
> > It should for sure not be done in imx-uart specific code. But I think
> > that deasserting RTS before calling .set_termios (iff rtscts is enabled)
> > is a sane thing to do for generic code. I don't want to motivate the
> > other side to send data while I reconfigure my receiver. Yes, this is a
> > visible change, but IMHO a good one.
> >
> >> Correct changing of baud rates, bits, etc., could only be implemented at
> >> communication protocol level (read: application level), and there are
> >> all the tools in the kernel to do it right, provided driver does not do
> >> what it was not asked to do.
> >
> > Hmm, deasserting RTS while not being ready helps here. Otherwise the
> > communication partner that sends first after both agreed to change the
> > baud rate might start doing that before the receiver on the other end is
> > done. When RTS is deasserted this race window is at least smaller.
> 
> In general, it's a wrong idea to do in the kernel what could be done as
> efficiently at application level.

I agree a bit here. If the resulting behaviour when relying on userspace
might be wrong, the kernel should be free to fix (or smooth) it. (Even a
WARN_ON_ONCE would be fine in my eyes.) But I don't care enough to
discuss this further. The behaviour with the patch under discussion is
better than before, so let's settle it with that.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Russell King - ARM Linux admin]

On Mon, Jul 22, 2019 at 09:51:07AM +0200, Uwe Kleine-König wrote:
> On Mon, Jul 22, 2019 at 10:42:57AM +0300, Sergey Organov wrote:
> > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > 
> > > On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
> > >> Hello Uwe,
> > >> 
> > >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > >> > Hello Sergey,
> > >> >
> > >> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
> > >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > >> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
> > >> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > >> >> >> index 57d6e6b..95d7984 100644
> > >> >> >> --- a/drivers/tty/serial/imx.c
> > >> >> >> +++ b/drivers/tty/serial/imx.c
> > >> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
> > >> >> >>  /* called with port.lock taken and irqs caller dependent */
> > >> >> >>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
> > >> >> >>  {
> > >> >> >> -	*ucr2 |= UCR2_CTSC;
> > >> >> >> +	if (*ucr2 & UCR2_CTS)
> > >> >> >> +		*ucr2 |= UCR2_CTSC;
> > >> >> >
> > >> >> > I think this patch is wrong or the commit log is insufficient.
> > >> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
> > >> >> > never true. And CTSC isn't restored anywhere, is it?
> > >> >> 
> > >> >> This is rebase to 'tty-next' branch, and you need to look at it in that
> > >> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
> > >> >> fix that is already there.
> > >> >
> > >> > I looked at 57d6e6b which is the file you patched. And there
> > >> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
> > >> >
> > >> > If you still think I'm wrong, please improve the commit log
> > >> > accordingly.
> > >> 
> > >> I still think you are wrong, but I don't know how to improve commit log.
> > >> 
> > >> To check it once again, I just did:
> > >> 
> > >> $ git show 57d6e6b > imx.c
> > >> 
> > >> There, in imx_uart_set_termios(), I see:
> > >> 
> > >> 1569:	old_ucr2 = imx_uart_readl(sport, UCR2);
> > >> 1570:	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
> > >> 
> > >> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
> > >> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
> > >> 
> > >> Then, later in the same function:
> > >> 
> > >> 1591:		imx_uart_rts_auto(sport, &ucr2);
> > >> 
> > >> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
> > >> 
> > >> That's what the patch does, checks this bit.
> > >> 
> > >> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
> > >> UCR2_CTS". Do we maybe still read different versions of the file?
> > >
> > > No, it's just that I failed to see that UCR2_CTS is in the set of bits
> > > that are retained even when looking twice :-|
> > 
> > Ah, that one... How familiar :-)
> 
> I thought again a bit over the weekend about this. I wonder if it's
> correct to keep RTS active while going through .set_termios. Shouldn't
> it maybe always be inactive to prevent the other side sending data while
> we are changing the baud rate?

Only if CRTSCTS is enabled, and then there are a lot of serial drivers
to fix.

However, RTS is not guaranteed to stop the remote end sending characters
as soon as it is deasserted - 16550 relies on software noticing that
CTS has changed, and even then it may have a FIFO full of characters
already queued to be sent that will still be sent.

So, disabling RTS just before changing the baud doesn't give any
guarantees that a character won't be in the process of being received
while we're changing the baud rate, which means disabling it doesn't
actually gain us anything.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

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

Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Uwe Kleine-König]

Hello Russell,

On Mon, Jul 22, 2019 at 10:57:21AM +0100, Russell King - ARM Linux admin wrote:
> On Mon, Jul 22, 2019 at 09:51:07AM +0200, Uwe Kleine-König wrote:
> > On Mon, Jul 22, 2019 at 10:42:57AM +0300, Sergey Organov wrote:
> > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > > 
> > > > On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
> > > >> Hello Uwe,
> > > >> 
> > > >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > > >> > Hello Sergey,
> > > >> >
> > > >> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
> > > >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > > >> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
> > > >> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > >> >> >> index 57d6e6b..95d7984 100644
> > > >> >> >> --- a/drivers/tty/serial/imx.c
> > > >> >> >> +++ b/drivers/tty/serial/imx.c
> > > >> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
> > > >> >> >>  /* called with port.lock taken and irqs caller dependent */
> > > >> >> >>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
> > > >> >> >>  {
> > > >> >> >> -	*ucr2 |= UCR2_CTSC;
> > > >> >> >> +	if (*ucr2 & UCR2_CTS)
> > > >> >> >> +		*ucr2 |= UCR2_CTSC;
> > > >> >> >
> > > >> >> > I think this patch is wrong or the commit log is insufficient.
> > > >> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
> > > >> >> > never true. And CTSC isn't restored anywhere, is it?
> > > >> >> 
> > > >> >> This is rebase to 'tty-next' branch, and you need to look at it in that
> > > >> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
> > > >> >> fix that is already there.
> > > >> >
> > > >> > I looked at 57d6e6b which is the file you patched. And there
> > > >> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
> > > >> >
> > > >> > If you still think I'm wrong, please improve the commit log
> > > >> > accordingly.
> > > >> 
> > > >> I still think you are wrong, but I don't know how to improve commit log.
> > > >> 
> > > >> To check it once again, I just did:
> > > >> 
> > > >> $ git show 57d6e6b > imx.c
> > > >> 
> > > >> There, in imx_uart_set_termios(), I see:
> > > >> 
> > > >> 1569:	old_ucr2 = imx_uart_readl(sport, UCR2);
> > > >> 1570:	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
> > > >> 
> > > >> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
> > > >> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
> > > >> 
> > > >> Then, later in the same function:
> > > >> 
> > > >> 1591:		imx_uart_rts_auto(sport, &ucr2);
> > > >> 
> > > >> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
> > > >> 
> > > >> That's what the patch does, checks this bit.
> > > >> 
> > > >> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
> > > >> UCR2_CTS". Do we maybe still read different versions of the file?
> > > >
> > > > No, it's just that I failed to see that UCR2_CTS is in the set of bits
> > > > that are retained even when looking twice :-|
> > > 
> > > Ah, that one... How familiar :-)
> > 
> > I thought again a bit over the weekend about this. I wonder if it's
> > correct to keep RTS active while going through .set_termios. Shouldn't
> > it maybe always be inactive to prevent the other side sending data while
> > we are changing the baud rate?
> 
> Only if CRTSCTS is enabled, and then there are a lot of serial drivers
> to fix.

Ack, I included this condition in a later mail already.

> However, RTS is not guaranteed to stop the remote end sending characters
> as soon as it is deasserted - 16550 relies on software noticing that
> CTS has changed, and even then it may have a FIFO full of characters
> already queued to be sent that will still be sent.
> 
> So, disabling RTS just before changing the baud doesn't give any
> guarantees that a character won't be in the process of being received
> while we're changing the baud rate, which means disabling it doesn't
> actually gain us anything.

<sarcasm>With that reasoning we can drop RTS driving completely. Let's
just assert it unconditionally.</sarcam>

Right, deasserting RTS doesn't help against long receive FIFOs or
senders that react slowly (or not at all), but still it's better than
nothing, isn't it?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Russell King - ARM Linux admin]

On Mon, Jul 22, 2019 at 12:04:14PM +0200, Uwe Kleine-König wrote:
> Hello Russell,
> 
> On Mon, Jul 22, 2019 at 10:57:21AM +0100, Russell King - ARM Linux admin wrote:
> > However, RTS is not guaranteed to stop the remote end sending characters
> > as soon as it is deasserted - 16550 relies on software noticing that
> > CTS has changed, and even then it may have a FIFO full of characters
> > already queued to be sent that will still be sent.
> > 
> > So, disabling RTS just before changing the baud doesn't give any
> > guarantees that a character won't be in the process of being received
> > while we're changing the baud rate, which means disabling it doesn't
> > actually gain us anything.
> 
> <sarcasm>With that reasoning we can drop RTS driving completely. Let's
> just assert it unconditionally.</sarcam>

Please, I'm being serious.

> Right, deasserting RTS doesn't help against long receive FIFOs or
> senders that react slowly (or not at all), but still it's better than
> nothing, isn't it?

Not really.

In the normal use case of RTS, RTS doesn't get deasserted when there is
no buffer space available, but when the available buffer space reaches
a low-threshold.  Buffer space remains to allow the sender to react to
the change of RTS state.

In the case you are promoting, which is to deassert RTS and then
immediately start changing the port settings, you are not giving any
chance for the sender to react to that state change.  You could even
be mid-way through receiving a character from the remote end - and
at 75 baud, a single character lasts around 133ms.

Even if you wait 133ms, that doesn't mean that the remote end has
finished sending.  If the remote end has a 16 byte FIFO, you'd need
to wait about 2.2 seconds.  At faster baud rates, of course the
delay gets shorter.

Just deasserting RTS just before changing the port settings gains
very little protection.  You need a character-period based delay as
well.

If we do start adding delays, it means that changing the baud rate
for a port set to 75 baud starts taking ages to complete if CRTSCTS
is enabled, irrespective of the settings change mode that was
requested by userspace.

However, adding delays is likely to screw up various userspace
applications, such as those that do need to change baud rate at
specific points in their protocol.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

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

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

[Sergey Organov]

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.

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 95d7984..34d61c4 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -970,10 +970,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);
 	}
 
-- 
2.10.0.1.g57b01a3


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

[PATCH v4 3/3] serial: imx: get rid of imx_uart_rts_auto()

[Sergey Organov]

Called in only one place, for RS232, it only obscures things, as it
doesn't go well with 2 similar named functions,
imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
RS485-specific.

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 | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 34d61c4..971055b 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -402,13 +402,6 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
-/* called with port.lock taken and irqs caller dependent */
-static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
-{
-	if (*ucr2 & UCR2_CTS)
-		*ucr2 |= UCR2_CTSC;
-}
-
 /* called with port.lock taken and irqs off */
 static void imx_uart_start_rx(struct uart_port *port)
 {
@@ -1600,8 +1593,10 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 		else
 			imx_uart_rts_inactive(sport, &ucr2);
 
-	} else if (termios->c_cflag & CRTSCTS)
-		imx_uart_rts_auto(sport, &ucr2);
+	} else if (termios->c_cflag & CRTSCTS) {
+		if (ucr2 & UCR2_CTS)
+			ucr2 |= UCR2_CTSC;
+	}
 
 	if (termios->c_cflag & CRTSCTS)
 		ucr2 &= ~UCR2_IRTS;
-- 
2.10.0.1.g57b01a3


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

[PATCH v5 0/3] serial: imx: fix RTS and RTS/CTS handling

[Sergey Organov]

This is rebase of v3 on top of 'tty-next', to get rid of commits that
are already adopted to mainline.

RTS signal and RTS/CTS handshake handling had a few problems these
patches fix.

In addition, minor cleanups are made to the involved code.

Changelog:

  v5:

     * improved commit description and added more comments for
       "serial: imx: set_termios(): do not enable autoRTS if RTS is
       unset" as suggested by
       Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
       and added corresponding "Reviewed-by:"

  v4:

      * rebased on top of 'tty-next', to get rid of commits that
        are already adopted to mainline.

  v3:

      * Improved comments in "serial: imx: set_mctrl(): correctly
        restore autoRTS state", as suggested by Uwe Kleine-König
        <u.kleine-koenig@pengutronix.de>

  v2:

      * Appended: "Reviewed-by:" and "Tested-by:"
        Sascha Hauer <s.hauer@pengutronix.de>

      * Removed "RFC" from header

  v1:

      * Fixed in "serial: imx: set_termios(): preserve RTS state"

-+	ucr2 = UCR2_SRST | UCR2_IRTS;
++	ucr2 |= UCR2_SRST | UCR2_IRTS;

        as noticed by Lothar Waßmann <LW@KARO-electronics.de>

      * Fixed in "serial: imx: set_termios(): preserve RTS state"

-+	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
++	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);

        as the fix for the problem found by Sascha Hauer
        <s.hauer@pengutronix.de>

      * Reordered:

        serial: imx: set_termios(): preserve RTS state
        serial: imx: set_termios(): do not enable autoRTS if RTS is unset

        as the latter makes sense only provided the former is already
        applied.

Sergey Organov (3):
  serial: imx: set_termios(): do not enable autoRTS if RTS is unset
  serial: imx: set_mctrl(): correctly restore autoRTS state
  serial: imx: get rid of imx_uart_rts_auto()

 drivers/tty/serial/imx.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

--
2.10.0.1.g57b01a3


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

[PATCH v5 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

Don't let receiver hardware automatically control RTS output if it
was requested to be inactive.

To ensure this, set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS
(=TIOCM_RTS) is cleared. Added corresponding check in imx_uart_rts_auto()
to fix this.

Reviewed-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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 57d6e6b..32f36d8 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -405,7 +405,12 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 /* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
 {
-	*ucr2 |= UCR2_CTSC;
+	/*
+	 * 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;
 }
 
 /* called with port.lock taken and irqs off */
-- 
2.10.0.1.g57b01a3


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

Re: [PATCH v5 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Uwe Kleine-König]

On Mon, Jul 22, 2019 at 12:22:08PM +0300, Sergey Organov wrote:
> Don't let receiver hardware automatically control RTS output if it
> was requested to be inactive.
> 
> To ensure this, set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS
> (=TIOCM_RTS) is cleared. Added corresponding check in imx_uart_rts_auto()
> to fix this.
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I think it's a bit bold to add a review-tag for me here. The best reason
for that that I'm aware of is that I wrote for v4: "[I]f you update the
commit log as agreed already before and even add a comment in
imx_uart_rts_auto along the lines of ... you can have my Ack." which IMO
isn't good enough to justify a "Reviewed-by". I wouldn't even add an
Acked-by: without the other person being able to actually see the
changed patch (but this might be arguable).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v5 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

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

> On Mon, Jul 22, 2019 at 12:22:08PM +0300, Sergey Organov wrote:
>> Don't let receiver hardware automatically control RTS output if it
>> was requested to be inactive.
>> 
>> To ensure this, set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS
>> (=TIOCM_RTS) is cleared. Added corresponding check in imx_uart_rts_auto()
>> to fix this.
>> 
>> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> I think it's a bit bold to add a review-tag for me here. The best reason
> for that that I'm aware of is that I wrote for v4: "[I]f you update the
> commit log as agreed already before and even add a comment in
> imx_uart_rts_auto along the lines of ... you can have my Ack." which IMO
> isn't good enough to justify a "Reviewed-by". I wouldn't even add an
> Acked-by: without the other person being able to actually see the
> changed patch (but this might be arguable).

I'll do whatever you say. Should I remove the Reviewed-by: you, or is it
OK to leave it in, to avoid re-iterating again?

Best Regards,

-- Sergey

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

Re: [PATCH v5 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Uwe Kleine-König]

On Mon, Jul 22, 2019 at 04:57:49PM +0300, Sergey Organov wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> 
> > On Mon, Jul 22, 2019 at 12:22:08PM +0300, Sergey Organov wrote:
> >> Don't let receiver hardware automatically control RTS output if it
> >> was requested to be inactive.
> >> 
> >> To ensure this, set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS
> >> (=TIOCM_RTS) is cleared. Added corresponding check in imx_uart_rts_auto()
> >> to fix this.
> >> 
> >> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >
> > I think it's a bit bold to add a review-tag for me here. The best reason
> > for that that I'm aware of is that I wrote for v4: "[I]f you update the
> > commit log as agreed already before and even add a comment in
> > imx_uart_rts_auto along the lines of ... you can have my Ack." which IMO
> > isn't good enough to justify a "Reviewed-by". I wouldn't even add an
> > Acked-by: without the other person being able to actually see the
> > changed patch (but this might be arguable).
> 
> I'll do whatever you say. Should I remove the Reviewed-by: you, or is it
> OK to leave it in, to avoid re-iterating again?

I'd like to have it an Acked-by.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v5 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

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

> On Mon, Jul 22, 2019 at 04:57:49PM +0300, Sergey Organov wrote:
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> 
>> > On Mon, Jul 22, 2019 at 12:22:08PM +0300, Sergey Organov wrote:
>> >> Don't let receiver hardware automatically control RTS output if it
>> >> was requested to be inactive.
>> >> 
>> >> To ensure this, set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS
>> >> (=TIOCM_RTS) is cleared. Added corresponding check in imx_uart_rts_auto()
>> >> to fix this.
>> >> 
>> >> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> >
>> > I think it's a bit bold to add a review-tag for me here. The best reason
>> > for that that I'm aware of is that I wrote for v4: "[I]f you update the
>> > commit log as agreed already before and even add a comment in
>> > imx_uart_rts_auto along the lines of ... you can have my Ack." which IMO
>> > isn't good enough to justify a "Reviewed-by". I wouldn't even add an
>> > Acked-by: without the other person being able to actually see the
>> > changed patch (but this might be arguable).
>> 
>> I'll do whatever you say. Should I remove the Reviewed-by: you, or is it
>> OK to leave it in, to avoid re-iterating again?
>
> I'd like to have it an Acked-by.

OK, will do, thanks!

-- Sergey

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

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

[Sergey Organov]

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.

Reviewed-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.
+		 */
 		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);
 	}
 
-- 
2.10.0.1.g57b01a3


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

[PATCH v5 3/3] serial: imx: get rid of imx_uart_rts_auto()

[Sergey Organov]

Called in only one place, for RS232, it only obscures things, as it
doesn't go well with 2 similar named functions,
imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
RS485-specific.

Reviewed-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 | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 059ba35..d9a73c7 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -402,17 +402,6 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
-/* called with port.lock taken and irqs caller dependent */
-static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
-{
-	/*
-	 * 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;
-}
-
 /* called with port.lock taken and irqs off */
 static void imx_uart_start_rx(struct uart_port *port)
 {
@@ -1604,8 +1593,14 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 		else
 			imx_uart_rts_inactive(sport, &ucr2);
 
-	} else if (termios->c_cflag & CRTSCTS)
-		imx_uart_rts_auto(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;
+	}
 
 	if (termios->c_cflag & CRTSCTS)
 		ucr2 &= ~UCR2_IRTS;
-- 
2.10.0.1.g57b01a3


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

[PATCH v6 0/3] serial: imx: fix RTS and RTS/CTS handling

[Sergey Organov]

This is rebase of v3 on top of 'tty-next', to get rid of commits that
are already adopted to mainline.

RTS signal and RTS/CTS handshake handling had a few problems these
patches fix.

In addition, minor cleanups are made to the involved code.

Changelog:

  v6:

     * changed "Reviewed-by:" 
       Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
       to "Acked-by:" him, according to his request

  v5:

     * improved commit description and added more comments for
       "serial: imx: set_termios(): do not enable autoRTS if RTS is
       unset" as suggested by
       Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
       and added corresponding "Reviewed-by:"

  v4:

      * rebased on top of 'tty-next', to get rid of commits that
        are already adopted to mainline.

  v3:

      * Improved comments in "serial: imx: set_mctrl(): correctly
        restore autoRTS state", as suggested by Uwe Kleine-König
        <u.kleine-koenig@pengutronix.de>

  v2:

      * Appended: "Reviewed-by:" and "Tested-by:"
        Sascha Hauer <s.hauer@pengutronix.de>

      * Removed "RFC" from header

  v1:

      * Fixed in "serial: imx: set_termios(): preserve RTS state"

-+	ucr2 = UCR2_SRST | UCR2_IRTS;
++	ucr2 |= UCR2_SRST | UCR2_IRTS;

        as noticed by Lothar Waßmann <LW@KARO-electronics.de>

      * Fixed in "serial: imx: set_termios(): preserve RTS state"

-+	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
++	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);

        as the fix for the problem found by Sascha Hauer
        <s.hauer@pengutronix.de>

      * Reordered:

        serial: imx: set_termios(): preserve RTS state
        serial: imx: set_termios(): do not enable autoRTS if RTS is unset

        as the latter makes sense only provided the former is already
        applied.

Sergey Organov (3):
  serial: imx: set_termios(): do not enable autoRTS if RTS is unset
  serial: imx: set_mctrl(): correctly restore autoRTS state
  serial: imx: get rid of imx_uart_rts_auto()

 drivers/tty/serial/imx.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

-- 
2.10.0.1.g57b01a3


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

[PATCH v6 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

Don't let receiver hardware automatically control RTS output if it
was requested to be inactive.

To ensure this, set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS
(=TIOCM_RTS) is cleared. Added corresponding check in imx_uart_rts_auto()
to fix this.

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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 57d6e6b..32f36d8 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -405,7 +405,12 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 /* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
 {
-	*ucr2 |= UCR2_CTSC;
+	/*
+	 * 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;
 }
 
 /* called with port.lock taken and irqs off */
-- 
2.10.0.1.g57b01a3


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

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

[Sergey Organov]

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.
+		 */
 		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);
 	}
 
-- 
2.10.0.1.g57b01a3


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

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

[Uwe Kleine-König]

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

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

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

[Sergey Organov]

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

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

[Uwe Kleine-König]

On Tue, Jul 23, 2019 at 12:20:38PM +0300, Sergey Organov wrote:
> 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>

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

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

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

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

[Sergey Organov]

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

> On Tue, Jul 23, 2019 at 12:20:38PM +0300, Sergey Organov wrote:
>> 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>
>
> 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

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

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

[Sergey Organov]

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

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

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

[PATCH v6 3/3] serial: imx: get rid of imx_uart_rts_auto()

[Sergey Organov]

Called in only one place, for RS232, it only obscures things, as it
doesn't go well with 2 similar named functions,
imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
RS485-specific.

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 | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 059ba35..d9a73c7 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -402,17 +402,6 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
-/* called with port.lock taken and irqs caller dependent */
-static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
-{
-	/*
-	 * 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;
-}
-
 /* called with port.lock taken and irqs off */
 static void imx_uart_start_rx(struct uart_port *port)
 {
@@ -1604,8 +1593,14 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 		else
 			imx_uart_rts_inactive(sport, &ucr2);
 
-	} else if (termios->c_cflag & CRTSCTS)
-		imx_uart_rts_auto(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;
+	}
 
 	if (termios->c_cflag & CRTSCTS)
 		ucr2 &= ~UCR2_IRTS;
-- 
2.10.0.1.g57b01a3


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

[PATCH v7 0/3] serial: imx: fix RTS and RTS/CTS handling

[Sergey Organov]

This is rebase of v3 on top of 'tty-next', to get rid of commits that
are already adopted to mainline.

RTS signal and RTS/CTS handshake handling had a few problems these
patches fix.

In addition, minor cleanups are made to the involved code.

Changelog:

  v7:

     * removed "Acked-by:" Uwe Kleine-König from
       two last patches where they were put by mistake.

  v6:

     * changed "Reviewed-by:"
       Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
       to "Acked-by:" him, according to his request

  v5:

     * improved commit description and added more comments for
       "serial: imx: set_termios(): do not enable autoRTS if RTS is
       unset" as suggested by
       Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
       and added corresponding "Reviewed-by:"

  v4:

      * rebased on top of 'tty-next', to get rid of commits that
        are already adopted to mainline.

  v3:

      * Improved comments in "serial: imx: set_mctrl(): correctly
        restore autoRTS state", as suggested by Uwe Kleine-König
        <u.kleine-koenig@pengutronix.de>

  v2:

      * Appended: "Reviewed-by:" and "Tested-by:"
        Sascha Hauer <s.hauer@pengutronix.de>

      * Removed "RFC" from header

  v1:

      * Fixed in "serial: imx: set_termios(): preserve RTS state"

-+	ucr2 = UCR2_SRST | UCR2_IRTS;
++	ucr2 |= UCR2_SRST | UCR2_IRTS;

        as noticed by Lothar Waßmann <LW@KARO-electronics.de>

      * Fixed in "serial: imx: set_termios(): preserve RTS state"

-+	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
++	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);

        as the fix for the problem found by Sascha Hauer
        <s.hauer@pengutronix.de>

      * Reordered:

        serial: imx: set_termios(): preserve RTS state
        serial: imx: set_termios(): do not enable autoRTS if RTS is unset

        as the latter makes sense only provided the former is already
        applied.

Sergey Organov (3):
  serial: imx: set_termios(): do not enable autoRTS if RTS is unset
  serial: imx: set_mctrl(): correctly restore autoRTS state
  serial: imx: get rid of imx_uart_rts_auto()

 drivers/tty/serial/imx.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

--
2.10.0.1.g57b01a3


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

[PATCH v7 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Sergey Organov]

Don't let receiver hardware automatically control RTS output if it
was requested to be inactive.

To ensure this, set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS
(=TIOCM_RTS) is cleared. Added corresponding check in imx_uart_rts_auto()
to fix this.

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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 57d6e6b..32f36d8 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -405,7 +405,12 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 /* called with port.lock taken and irqs caller dependent */
 static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
 {
-	*ucr2 |= UCR2_CTSC;
+	/*
+	 * 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;
 }
 
 /* called with port.lock taken and irqs off */
-- 
2.10.0.1.g57b01a3


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

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

[Sergey Organov]

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.

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.
+		 */
 		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);
 	}
 
-- 
2.10.0.1.g57b01a3


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

[PATCH v7 3/3] serial: imx: get rid of imx_uart_rts_auto()

[Sergey Organov]

Called in only one place, for RS232, it only obscures things, as it
doesn't go well with 2 similar named functions,
imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
RS485-specific.

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 | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 059ba35..d9a73c7 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -402,17 +402,6 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
 	mctrl_gpio_set(sport->gpios, sport->port.mctrl);
 }
 
-/* called with port.lock taken and irqs caller dependent */
-static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
-{
-	/*
-	 * 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;
-}
-
 /* called with port.lock taken and irqs off */
 static void imx_uart_start_rx(struct uart_port *port)
 {
@@ -1604,8 +1593,14 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 		else
 			imx_uart_rts_inactive(sport, &ucr2);
 
-	} else if (termios->c_cflag & CRTSCTS)
-		imx_uart_rts_auto(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;
+	}
 
 	if (termios->c_cflag & CRTSCTS)
 		ucr2 &= ~UCR2_IRTS;
-- 
2.10.0.1.g57b01a3


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

Re: [PATCH v7 3/3] serial: imx: get rid of imx_uart_rts_auto()

[Uwe Kleine-König]

On Fri, Jul 26, 2019 at 09:52:41PM +0300, Sergey Organov wrote:
> Called in only one place, for RS232, it only obscures things, as it
> doesn't go well with 2 similar named functions,
> imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
> RS485-specific.

I don't share the critic. IMHO the name is fine. imx_uart_rts_inactive
sets rts to its inactive level, imx_uart_rts_active() to its active
level and imx_uart_rts_auto() lets the output drive automatically by the
receiver.

The name started to be a bit wrong in patch 1 of the series however.

And I still object removing this function because with the semantic this
function got in patch 1 it is suiteable to be used in
imx_uart_set_mctrl().

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH v7 3/3] serial: imx: get rid of imx_uart_rts_auto()

[Sergey Organov]

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

> On Fri, Jul 26, 2019 at 09:52:41PM +0300, Sergey Organov wrote:
>> Called in only one place, for RS232, it only obscures things, as it
>> doesn't go well with 2 similar named functions,
>> imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
>> RS485-specific.
>
> I don't share the critic. IMHO the name is fine. imx_uart_rts_inactive
> sets rts to its inactive level,
> imx_uart_rts_active() to its active level

Not exactly, in fact both do more than that, in a similar manner.

> imx_uart_rts_auto() lets the output drive automatically by the
> receiver.

And this one was different and it was rather confusing when I've tried
to grok the logic of the driver.

> The name started to be a bit wrong in patch 1 of the series however.

The function was different from first two even before the patch, as it
does not do any of those additional things the first two do.

> And I still object removing this function because with the semantic
> this function got in patch 1 it is suiteable to be used in
> imx_uart_set_mctrl().

It is not, as it does require change to be used there, as we've already
seen, and then it becomes very different function from what it was at
the beginning.

Even then, the end result I've shown you when attempting to somehow preserve
some re-incarnation of this function still seems more cumbersome to me
than the end result of these patches.

That said, this a matter of taste and style, not correctness, and could
be changed as a follow-up, not to risk breaking already tested patch
series.

Thanks,

-- Sergey

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

Re: [PATCH v7 3/3] serial: imx: get rid of imx_uart_rts_auto()

[Uwe Kleine-König]

On Mon, Jul 29, 2019 at 12:03:07PM +0300, Sergey Organov wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> 
> > On Fri, Jul 26, 2019 at 09:52:41PM +0300, Sergey Organov wrote:
> >> Called in only one place, for RS232, it only obscures things, as it
> >> doesn't go well with 2 similar named functions,
> >> imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
> >> RS485-specific.
> >
> > I don't share the critic. IMHO the name is fine. imx_uart_rts_inactive
> > sets rts to its inactive level,
> > imx_uart_rts_active() to its active level
> 
> Not exactly, in fact both do more than that, in a similar manner.

They both handle mctrl-gpio, the autorts stuff isn't available for that,
so we could fix that by letting rts-auto set the RTS gpio to active.

> > imx_uart_rts_auto() lets the output drive automatically by the
> > receiver.
> 
> And this one was different and it was rather confusing when I've tried
> to grok the logic of the driver.
> 
> > The name started to be a bit wrong in patch 1 of the series however.
> 
> The function was different from first two even before the patch, as it
> does not do any of those additional things the first two do.
> 
> > And I still object removing this function because with the semantic
> > this function got in patch 1 it is suiteable to be used in
> > imx_uart_set_mctrl().
> 
> It is not, as it does require change to be used there, as we've already
> seen, and then it becomes very different function from what it was at
> the beginning.
> 
> Even then, the end result I've shown you when attempting to somehow preserve
> some re-incarnation of this function still seems more cumbersome to me
> than the end result of these patches.
> 
> That said, this a matter of taste and style, not correctness, and could
> be changed as a follow-up, not to risk breaking already tested patch
> series.

*shrug* I stop caring here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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