* [PATCH 0/2] serial: imx: use UPF_AUTO_CTS @ 2019-06-26 10:15 Sascha Hauer 2019-06-26 10:15 ` [PATCH 1/2] serial: imx: remove duplicate handling of CTS change Sascha Hauer 2019-06-26 10:15 ` [PATCH 2/2] serial: imx: use UPF_AUTO_CTS Sascha Hauer 0 siblings, 2 replies; 5+ messages in thread From: Sascha Hauer @ 2019-06-26 10:15 UTC (permalink / raw) To: linux-serial Cc: Pengutronix Kernel Team, Sascha Hauer, Sergey Organov, NXP Linux Team, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] serial: imx: remove duplicate handling of CTS change 2019-06-26 10:15 [PATCH 0/2] serial: imx: use UPF_AUTO_CTS Sascha Hauer @ 2019-06-26 10:15 ` Sascha Hauer 2019-06-27 6:16 ` Uwe Kleine-König 2019-06-26 10:15 ` [PATCH 2/2] serial: imx: use UPF_AUTO_CTS Sascha Hauer 1 sibling, 1 reply; 5+ messages in thread From: Sascha Hauer @ 2019-06-26 10:15 UTC (permalink / raw) To: linux-serial Cc: Pengutronix Kernel Team, Sascha Hauer, Sergey Organov, NXP Linux Team, linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] serial: imx: remove duplicate handling of CTS change 2019-06-26 10:15 ` [PATCH 1/2] serial: imx: remove duplicate handling of CTS change Sascha Hauer @ 2019-06-27 6:16 ` Uwe Kleine-König 2019-06-27 7:59 ` Sascha Hauer 0 siblings, 1 reply; 5+ messages in thread From: Uwe Kleine-König @ 2019-06-27 6:16 UTC (permalink / raw) To: Sascha Hauer Cc: Pengutronix Kernel Team, Sergey Organov, linux-arm-kernel, linux-serial, NXP Linux Team 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] serial: imx: remove duplicate handling of CTS change 2019-06-27 6:16 ` Uwe Kleine-König @ 2019-06-27 7:59 ` Sascha Hauer 0 siblings, 0 replies; 5+ messages in thread From: Sascha Hauer @ 2019-06-27 7:59 UTC (permalink / raw) To: Uwe Kleine-König Cc: Pengutronix Kernel Team, Sergey Organov, linux-arm-kernel, linux-serial, NXP Linux Team 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] serial: imx: use UPF_AUTO_CTS 2019-06-26 10:15 [PATCH 0/2] serial: imx: use UPF_AUTO_CTS Sascha Hauer 2019-06-26 10:15 ` [PATCH 1/2] serial: imx: remove duplicate handling of CTS change Sascha Hauer @ 2019-06-26 10:15 ` Sascha Hauer 1 sibling, 0 replies; 5+ messages in thread From: Sascha Hauer @ 2019-06-26 10:15 UTC (permalink / raw) To: linux-serial Cc: Pengutronix Kernel Team, Sascha Hauer, Sergey Organov, NXP Linux Team, linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-27 7:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-26 10:15 [PATCH 0/2] serial: imx: use UPF_AUTO_CTS Sascha Hauer 2019-06-26 10:15 ` [PATCH 1/2] serial: imx: remove duplicate handling of CTS change Sascha Hauer 2019-06-27 6:16 ` Uwe Kleine-König 2019-06-27 7:59 ` Sascha Hauer 2019-06-26 10:15 ` [PATCH 2/2] serial: imx: use UPF_AUTO_CTS Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).