On Wed, Sep 20, 2017 at 11:58:51AM +0200, Uwe Kleine-König wrote:
> On Thu, Sep 14, 2017 at 08:03:28AM +0200, Uwe Kleine-König wrote:
> > Hello Ian,
> >
> > On Wed, Sep 13, 2017 at 11:26:02PM +0100, Ian Arkver wrote:
> > > On 13/09/17 22:11, Uwe Kleine-König wrote:
> > > > RS485 uses a single differential signal pair for both sending and
> > > > receiving. So there is an additional signal necessary to switch between
> > > > sending and receiving. Usually this signal is RTS, there are a few
> > > > pieces of evidence:
> > > >
> > > > - the rs485 device tree binding calls this signal RTS;
> > > > - all drivers I checked use RTS for direction switching
> > > > (atmel_serial, crisv10, imx, fsl_lpuart, omap-serial, sc16is7xx.c);
> > > > - nearly all machines I saw called this signal RTS in their schematic;
> > > > and
> > > > - the members of struct serial_rs485 and related defines use RTS in
> > > > their names.
> > > >
> > > > With such a setup using the imx driver I have the problem that opening a
> > > > serial device that is in RS485 mode for reading enables RTS via:
> > > >
> > > > tty_port_open() -> tty_port_block_til_ready() ->
> > > > tty_port_raise_dtr_rts() -> uart_dtr_rts() ->
> > > > uart_update_mctrl()
> > > >
> > > > and so disables receiving. The imx driver supports two different ways to
> > > > drive RTS: a) a native mode using UCR2_CTS and UCR2_CTSC (Freescale
> > > > named the UART signals from the modem POV, so what we call RTS is CTS in
> > > > the registers) and b) a gpio.
> > > >
> > > > For case a) imx_set_mctrl already ignores TIOCM_RTS if the uart is in
> > > > RS485 mode. For case b) however it doesn't and so data may be missed to
> > > > read.
> > > >
> > > > As I expect all drivers have the same problem it makes sense to fix this
> > > > in serial_core instead of teaching the imx driver to also ignore RTS for
> > > > the gpio case.
> > > >
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > > > ---
> > > > drivers/tty/serial/serial_core.c | 12 ++++++++++--
> > > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > > > index 2b74ad9bfc64..8a52f28cf1ce 100644
> > > > --- a/drivers/tty/serial/serial_core.c
> > > > +++ b/drivers/tty/serial/serial_core.c
> > > > @@ -1662,15 +1662,23 @@ static void uart_dtr_rts(struct tty_port *port, int onoff)
> > > > {
> > > > struct uart_state *state = container_of(port, struct uart_state, port);
> > > > struct uart_port *uport;
> > > > + unsigned int dtrrts = TIOCM_DTR | TIOCM_RTS;
> > > > uport = uart_port_ref(state);
> > > > if (!uport)
> > > > return;
> > > > + /*
> > > > + * If RS485 is active, don't mess with RTS because that is supposed to
> > > > + * enable the transmitter
> > > > + */
> > > > + if (uport->rs485.flags & SER_RS485_ENABLED)
> > > > + dtrrts &= ~TIOCM_RTS;
> > > > +
> > > > if (onoff)
> > > > - uart_set_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
> > > > + uart_set_mctrl(uport, dtrrts);
> > > > else
> > > > - uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
> > > > + uart_clear_mctrl(uport, dtrrts);
> > > > uart_port_deref(uport);
> > > > }
> > >
> > > I haven't tested this but I think, from inspection...
> > >
> > > Although you've removed the TIOCM_RTS from the set/clear, these call
> > > uart_update_mctrl which uses a cached value of modem control lines in
> > > port->mctrl. This cached value is not updated by the imx.c functions
> > > imx_port_rts_active and imx_port_rts_inactive, which is a bit nasty.
> >
> > Right. I'm sure if you dig deeper you will find still more nasty corners
> > with rs485. :-\
> >
> > > If the uart is connected to a line driver with active low TX enable, the
> > > flag port->rs485.flags & SER_RS485_RTS_AFTER_SEND would be set, so the RTS
> > > line should be high for receive, but the cached port->mctrl would not
> > > include TIOCM_RTS so uart_update_mctrl will still set it low.
> >
> > You're right. That is a problem in imx.c though and so orthogonal to
> > this patch. Having said that I don't don't feel confident with all the
> > handshaking and interactions with serial_core and the requirements of
> > POSIX.
> >
> > > I think for your patch to work in this case, the imx_port_rts_(in)active
> > > functions would need fixing to update the mctrl cache.
> >
> > My test setup still doesn't work completely, maybe this is part of the
> > remaining issue.
>
> My now working setup includes both patches, this RFC and the patch that
> Ian created for imx.c to modify the cache on RTS changes. So I think
> this patch is fine and would be happy to see it applied.
>
> Greg, what do you think? I assume I create a complete and clean uped
> series that makes it work for me?
FTR: This patch is obsoleted by
a6845e1e1b78 ("serial: core: Consider rs485 settings to drive RTS")
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
