[PATCH 0/2] serial: imx: use UPF_AUTO_CTS

[PATCH 0/2] serial: imx: use UPF_AUTO_CTS

[Sascha Hauer]

This series eliminates a duplicate call to uart_handle_cts_change() and
sets the UPF_AUTO_CTS flag for the i.MX UART driver.

Normally setting the UPF_AUTO_CTS flag should only be a little
optimization as the transmitter is no longer enabled/disabled with every
CTS change, here it fixes an issue which initially brought me to
implement this patch. I am working on uploading a firmware to a Marvell
bluetooth chip. During download it often happened that a CTS interrupt
was lost and the upload stalled forever. This patch fixes the issue
(without knowing why we lost CTS interrupts in the first place)

This series is based on Sergei Shtylyovs series "serial: imx: fix RTS
and RTS/CTS handling" and should be applied ontop of it.

Sascha Hauer (2):
  serial: imx: remove duplicate handling of CTS change
  serial: imx: use UPF_AUTO_CTS

 drivers/tty/serial/imx.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

-- 
2.20.1


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

[PATCH 1/2] serial: imx: remove duplicate handling of CTS change

[Sascha Hauer]

We have an interrupt for the CTS input (RTS in FSL speech). Its handler
calls uart_handle_cts_change(), so we shouldn't do this in
imx_uart_mctrl_check() again.

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

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index a5e80a028e83..0419a084c0ed 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -805,12 +805,8 @@ static void imx_uart_clear_rx_errors(struct imx_port *sport);
 static unsigned int imx_uart_get_hwmctrl(struct imx_port *sport)
 {
 	unsigned int tmp = TIOCM_DSR;
-	unsigned usr1 = imx_uart_readl(sport, USR1);
 	unsigned usr2 = imx_uart_readl(sport, USR2);
 
-	if (usr1 & USR1_RTSS)
-		tmp |= TIOCM_CTS;
-
 	/* in DCE mode DCDIN is always 0 */
 	if (!(usr2 & USR2_DCDIN))
 		tmp |= TIOCM_CAR;
@@ -843,8 +839,6 @@ static void imx_uart_mctrl_check(struct imx_port *sport)
 		sport->port.icount.dsr++;
 	if (changed & TIOCM_CAR)
 		uart_handle_dcd_change(&sport->port, status & TIOCM_CAR);
-	if (changed & TIOCM_CTS)
-		uart_handle_cts_change(&sport->port, status & TIOCM_CTS);
 
 	wake_up_interruptible(&sport->port.state->port.delta_msr_wait);
 }
-- 
2.20.1


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

[PATCH 2/2] serial: imx: use UPF_AUTO_CTS

[Sascha Hauer]

The i.MX driver doesn't set the UPF_AUTO_CTS flag which means that
uart_handle_cts_change() will stop/start the receiver on CTS changes.
This is completely unnecessary as the hardware will handle CTS
changes automatically.

With UPF_AUTO_CTS enabled uart_handle_cts_change() boils down to
increasing the CTS statistic counter. For clarity inline increasing
the counter instead of calling uart_handle_cts_change().

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

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 0419a084c0ed..82f987dab066 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -703,13 +703,11 @@ static void imx_uart_start_tx(struct uart_port *port)
 static irqreturn_t imx_uart_rtsint(int irq, void *dev_id)
 {
 	struct imx_port *sport = dev_id;
-	u32 usr1;
 
 	spin_lock(&sport->port.lock);
 
 	imx_uart_writel(sport, USR1_RTSD, USR1);
-	usr1 = imx_uart_readl(sport, USR1) & USR1_RTSS;
-	uart_handle_cts_change(&sport->port, !!usr1);
+	sport->port.icount.cts++;
 	wake_up_interruptible(&sport->port.state->port.delta_msr_wait);
 
 	spin_unlock(&sport->port.lock);
@@ -1588,6 +1586,9 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	} else if (termios->c_cflag & CRTSCTS) {
 		if (ucr2 & UCR2_CTS)
 			ucr2 |= UCR2_CTSC;
+		port->status |= UPSTAT_AUTOCTS;
+	} else {
+		port->status &= ~UPSTAT_AUTOCTS;
 	}
 
 	if (termios->c_cflag & CRTSCTS)
@@ -1706,6 +1707,9 @@ static void imx_uart_config_port(struct uart_port *port, int flags)
 
 	if (flags & UART_CONFIG_TYPE)
 		sport->port.type = PORT_IMX;
+
+	if (sport->have_rtscts)
+		sport->port.flags |= UPF_AUTO_CTS;
 }
 
 /*
-- 
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 1/2] serial: imx: remove duplicate handling of CTS change

[Uwe Kleine-König]

On Wed, Jun 26, 2019 at 12:15:56PM +0200, Sascha Hauer wrote:
> We have an interrupt for the CTS input (RTS in FSL speech). Its handler
> calls uart_handle_cts_change(), so we shouldn't do this in
> imx_uart_mctrl_check() again.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/tty/serial/imx.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index a5e80a028e83..0419a084c0ed 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -805,12 +805,8 @@ static void imx_uart_clear_rx_errors(struct imx_port *sport);
>  static unsigned int imx_uart_get_hwmctrl(struct imx_port *sport)
>  {
>  	unsigned int tmp = TIOCM_DSR;
> -	unsigned usr1 = imx_uart_readl(sport, USR1);
>  	unsigned usr2 = imx_uart_readl(sport, USR2);
>  
> -	if (usr1 & USR1_RTSS)
> -		tmp |= TIOCM_CTS;
> -
>  	/* in DCE mode DCDIN is always 0 */
>  	if (!(usr2 & USR2_DCDIN))
>  		tmp |= TIOCM_CAR;

Is this hunk supposed to be included in this patch? I think it's wrong.

> @@ -843,8 +839,6 @@ static void imx_uart_mctrl_check(struct imx_port *sport)
>  		sport->port.icount.dsr++;
>  	if (changed & TIOCM_CAR)
>  		uart_handle_dcd_change(&sport->port, status & TIOCM_CAR);
> -	if (changed & TIOCM_CTS)
> -		uart_handle_cts_change(&sport->port, status & TIOCM_CTS);

This doesn't hurt, does it? Also imx_uart_mctrl_check is called from
imx_uart_timeout which is supposed to catch missed interrupts and in
this case uart_handle_cts_change() must be called.

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 1/2] serial: imx: remove duplicate handling of CTS change

[Sascha Hauer]

On Thu, Jun 27, 2019 at 08:16:07AM +0200, Uwe Kleine-König wrote:
> On Wed, Jun 26, 2019 at 12:15:56PM +0200, Sascha Hauer wrote:
> > We have an interrupt for the CTS input (RTS in FSL speech). Its handler
> > calls uart_handle_cts_change(), so we shouldn't do this in
> > imx_uart_mctrl_check() again.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/tty/serial/imx.c | 6 ------
> >  1 file changed, 6 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index a5e80a028e83..0419a084c0ed 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -805,12 +805,8 @@ static void imx_uart_clear_rx_errors(struct imx_port *sport);
> >  static unsigned int imx_uart_get_hwmctrl(struct imx_port *sport)
> >  {
> >  	unsigned int tmp = TIOCM_DSR;
> > -	unsigned usr1 = imx_uart_readl(sport, USR1);
> >  	unsigned usr2 = imx_uart_readl(sport, USR2);
> >  
> > -	if (usr1 & USR1_RTSS)
> > -		tmp |= TIOCM_CTS;
> > -
> >  	/* in DCE mode DCDIN is always 0 */
> >  	if (!(usr2 & USR2_DCDIN))
> >  		tmp |= TIOCM_CAR;
> 
> Is this hunk supposed to be included in this patch? I think it's wrong.

The rationale was that when we do not evaluate the TIOCM_CTS anymore in
the return value of imx_uart_get_hwmctrl() then there's no point in
setting it in the first place. However, imx_uart_get_hwmctrl() also has
another user which needs the flag, so right, this hunk shouldn't be
here.

> 
> > @@ -843,8 +839,6 @@ static void imx_uart_mctrl_check(struct imx_port *sport)
> >  		sport->port.icount.dsr++;
> >  	if (changed & TIOCM_CAR)
> >  		uart_handle_dcd_change(&sport->port, status & TIOCM_CAR);
> > -	if (changed & TIOCM_CTS)
> > -		uart_handle_cts_change(&sport->port, status & TIOCM_CTS);
> 
> This doesn't hurt, does it?

With this patch the number of CTS changes is correctly counted, I have
verified this with a logic analyzer. Without it port->icount.cts has 978
changes when it should be only 968 changes.

> Also imx_uart_mctrl_check is called from
> imx_uart_timeout which is supposed to catch missed interrupts and in
> this case uart_handle_cts_change() must be called.

Beginning with 2/2 uart_handle_cts_change() is needed for nothing else
but statistic counting. There won't be any timeout due to missed
interrupts as the hardware handles CTS itself.

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