serial: imx: support RS-485 Rx disable on Tx (2024)

Some RS-232 to RS-485 transceivers require Rx to be disabled on Tx toavoid echo of Tx data into the Rx buffer. Specifically, the XR3160ERS-232/RS-485/RS-422 transceiver behaves this way.This commit disables Rx on active Tx when SER_RS485_ENABLED is active andSER_RS485_RX_DURING_TX is disabled.Note that this is a change in behavior of the driver. Until nowSER_RS485_RX_DURING_TX was enabled unconditionally even when disabled inthe TIOCSRS485 ioctl serial_rs485 flags field.Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>Signed-off-by: Baruch Siach <baruch@tkos.co.il>--- drivers/tty/serial/imx.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

Hello Baruch,On Sun, Feb 28, 2016 at 11:25:51AM +0200, Baruch Siach wrote:> Some RS-232 to RS-485 transceivers require Rx to be disabled on Tx to> avoid echo of Tx data into the Rx buffer. Specifically, the XR3160E> RS-232/RS-485/RS-422 transceiver behaves this way.> > This commit disables Rx on active Tx when SER_RS485_ENABLED is active and> SER_RS485_RX_DURING_TX is disabled.> > Note that this is a change in behavior of the driver. Until nowBut this change is a good one (assuming it does what it advertises :-).Userspace got informed before that SER_RS485_RX_DURING_TX is enabled, sothis is not an incompatible change.Best regardsUwe> SER_RS485_RX_DURING_TX was enabled unconditionally even when disabled in> the TIOCSRS485 ioctl serial_rs485 flags field.> > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>> ---> drivers/tty/serial/imx.c | 5 ++++-> 1 file changed, 4 insertions(+), 1 deletion(-)> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c> index 9362f54c816c..333d34ff358c 100644> --- a/drivers/tty/serial/imx.c> +++ b/drivers/tty/serial/imx.c> @@ -361,6 +361,8 @@ static void imx_stop_tx(struct uart_port *port)> imx_port_rts_inactive(sport, &temp);> else> imx_port_rts_active(sport, &temp);> +if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))> +temp |= UCR2_RXEN;> writel(temp, port->membase + UCR2);> > temp = readl(port->membase + UCR4);> @@ -568,6 +570,8 @@ static void imx_start_tx(struct uart_port *port)> imx_port_rts_inactive(sport, &temp);> else> imx_port_rts_active(sport, &temp);> +if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))> +temp &= ~UCR2_RXEN;> writel(temp, port->membase + UCR2);Can this happen: - SER_RS485_RX_DURING_TX is off - thread A starts sending (and so disables RX) - thread B sets SER_RS485_RX_DURING_TX - thread A finishes sending, and doesn't restore RXEN.?Even if this cannot happen it might be more robust to restore RXENunconditionally in imx_stop_tx?!Best regardsUwe

Hi Uwe,Thanks for your prompt response.On Sun, Feb 28, 2016 at 10:56:01AM +0100, Uwe Kleine-König wrote:> On Sun, Feb 28, 2016 at 11:25:51AM +0200, Baruch Siach wrote:> > Some RS-232 to RS-485 transceivers require Rx to be disabled on Tx to> > avoid echo of Tx data into the Rx buffer. Specifically, the XR3160E> > RS-232/RS-485/RS-422 transceiver behaves this way.> > > > This commit disables Rx on active Tx when SER_RS485_ENABLED is active and> > SER_RS485_RX_DURING_TX is disabled.> > > > Note that this is a change in behavior of the driver. Until now> > But this change is a good one (assuming it does what it advertises :-).> Userspace got informed before that SER_RS485_RX_DURING_TX is enabled, so> this is not an incompatible change.I thought it is a good idea to mention this fact in the commit log anyway. It is not hard to imagine broken userspace being affected by this change.> > SER_RS485_RX_DURING_TX was enabled unconditionally even when disabled in> > the TIOCSRS485 ioctl serial_rs485 flags field.> > > > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>> > ---> > drivers/tty/serial/imx.c | 5 ++++-> > 1 file changed, 4 insertions(+), 1 deletion(-)> > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c> > index 9362f54c816c..333d34ff358c 100644> > --- a/drivers/tty/serial/imx.c> > +++ b/drivers/tty/serial/imx.c> > @@ -361,6 +361,8 @@ static void imx_stop_tx(struct uart_port *port)> > imx_port_rts_inactive(sport, &temp);> > else> > imx_port_rts_active(sport, &temp);> > +if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))> > +temp |= UCR2_RXEN;> > writel(temp, port->membase + UCR2);> > > > temp = readl(port->membase + UCR4);> > @@ -568,6 +570,8 @@ static void imx_start_tx(struct uart_port *port)> > imx_port_rts_inactive(sport, &temp);> > else> > imx_port_rts_active(sport, &temp);> > +if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))> > +temp &= ~UCR2_RXEN;> > writel(temp, port->membase + UCR2);> > Can this happen:> > - SER_RS485_RX_DURING_TX is off> - thread A starts sending (and so disables RX)> - thread B sets SER_RS485_RX_DURING_TX> - thread A finishes sending, and doesn't restore RXEN.> > ?> > Even if this cannot happen it might be more robust to restore RXEN> unconditionally in imx_stop_tx?!Sounds like a good idea. But if I take your comment to its logical conclusion, thread B might just disable SER_RS485_ENABLED entirely. Would it make sense to restore RXEN outside the 'if (port->rs485.flags & SER_RS485_ENABLED)' block? Or maybe we should just set RXEN in imx_rs485_config() when SER_RS485_RX_DURING_TX is enabled?baruch

Hello Baruch,On Sun, Feb 28, 2016 at 12:23:23PM +0200, Baruch Siach wrote:> Thanks for your prompt response.> > On Sun, Feb 28, 2016 at 10:56:01AM +0100, Uwe Kleine-König wrote:> > On Sun, Feb 28, 2016 at 11:25:51AM +0200, Baruch Siach wrote:> > > Some RS-232 to RS-485 transceivers require Rx to be disabled on Tx to> > > avoid echo of Tx data into the Rx buffer. Specifically, the XR3160E> > > RS-232/RS-485/RS-422 transceiver behaves this way.> > > > > > This commit disables Rx on active Tx when SER_RS485_ENABLED is active and> > > SER_RS485_RX_DURING_TX is disabled.> > > > > > Note that this is a change in behavior of the driver. Until now> > > > But this change is a good one (assuming it does what it advertises :-).> > Userspace got informed before that SER_RS485_RX_DURING_TX is enabled, so> > this is not an incompatible change.> > I thought it is a good idea to mention this fact in the commit log anyway. It > is not hard to imagine broken userspace being affected by this change.> > > > SER_RS485_RX_DURING_TX was enabled unconditionally even when disabled in> > > the TIOCSRS485 ioctl serial_rs485 flags field.> > > > > > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>> > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>> > > ---> > > drivers/tty/serial/imx.c | 5 ++++-> > > 1 file changed, 4 insertions(+), 1 deletion(-)> > > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c> > > index 9362f54c816c..333d34ff358c 100644> > > --- a/drivers/tty/serial/imx.c> > > +++ b/drivers/tty/serial/imx.c> > > @@ -361,6 +361,8 @@ static void imx_stop_tx(struct uart_port *port)> > > imx_port_rts_inactive(sport, &temp);> > > else> > > imx_port_rts_active(sport, &temp);> > > +if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))> > > +temp |= UCR2_RXEN;> > > writel(temp, port->membase + UCR2);> > > > > > temp = readl(port->membase + UCR4);> > > @@ -568,6 +570,8 @@ static void imx_start_tx(struct uart_port *port)> > > imx_port_rts_inactive(sport, &temp);> > > else> > > imx_port_rts_active(sport, &temp);> > > +if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))> > > +temp &= ~UCR2_RXEN;> > > writel(temp, port->membase + UCR2);> > > > Can this happen:> > > > - SER_RS485_RX_DURING_TX is off> > - thread A starts sending (and so disables RX)> > - thread B sets SER_RS485_RX_DURING_TX> > - thread A finishes sending, and doesn't restore RXEN.> > > > ?> > > > Even if this cannot happen it might be more robust to restore RXEN> > unconditionally in imx_stop_tx?!> > Sounds like a good idea. But if I take your comment to its logical conclusion, > thread B might just disable SER_RS485_ENABLED entirely. Would it make sense to > restore RXEN outside the 'if (port->rs485.flags & SER_RS485_ENABLED)' block? > Or maybe we should just set RXEN in imx_rs485_config() when > SER_RS485_RX_DURING_TX is enabled?The latter sounds like the right thing to do.Best regardsUwe

Patch

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.cindex 9362f54c816c..333d34ff358c 100644--- a/drivers/tty/serial/imx.c+++ b/drivers/tty/serial/imx.c@@ -361,6 +361,8 @@  static void imx_stop_tx(struct uart_port *port) imx_port_rts_inactive(sport, &temp); else imx_port_rts_active(sport, &temp);+if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))+temp |= UCR2_RXEN; writel(temp, port->membase + UCR2); temp = readl(port->membase + UCR4);@@ -568,6 +570,8 @@  static void imx_start_tx(struct uart_port *port) imx_port_rts_inactive(sport, &temp); else imx_port_rts_active(sport, &temp);+if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))+temp &= ~UCR2_RXEN; writel(temp, port->membase + UCR2); /* enable transmitter and shifter empty irq */@@ -1614,7 +1618,6 @@  static int imx_rs485_config(struct uart_port *port, /* unimplemented */ rs485conf->delay_rts_before_send = 0; rs485conf->delay_rts_after_send = 0;-rs485conf->flags |= SER_RS485_RX_DURING_TX; /* RTS is required to control the transmitter */ if (!sport->have_rtscts)