| Message ID | 3b5ba06fdb9c1bdd0b3018bf2f623f52b2856d18.1456651551.git.baruch@tkos.co.il (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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. Best regards Uwe > 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?! Best regards Uwe
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 regards Uwe
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); /* 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)
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 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(-)