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?
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
