| Message ID | 1560514294-29111-5-git-send-email-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | serial: imx: fix RTS and RTS/CTS handling | expand |
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
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.
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 */
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(-)