| Message ID | 1561558293-7683-7-git-send-email-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | serial: imx: fix RTS and RTS/CTS handling | expand |
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
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
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); }