Re: [PATCH] serial: Fix incorrect rs485 polarity on uart open

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, 2021-12-18 at 10:58 +0100, Lukas Wunner wrote:
Commit a6845e1e1b78 ("serial: core: Consider rs485 settings to drive
RTS") sought to deassert RTS when opening an rs485-enabled uart port.
That way, the transceiver does not occupy the bus until it transmits
data.

Unfortunately, the commit mixed up the logic and *asserted* RTS instead
of *deasserting* it:

The commit amended uart_port_dtr_rts(), which raises DTR and RTS when
opening an rs232 port.  "Raising" actually means lowering the signal
that's coming out of the uart, because an rs232 transceiver not only
changes a signal's voltage level, it also *inverts* the signal.  See
the simplified schematic in the MAX232 datasheet for an example:
https://www.ti.com/lit/ds/symlink/max232.pdf

So, to raise RTS on an rs232 port, TIOCM_RTS is *set* in port->mctrl
and that results in the signal being driven low.

In contrast to rs232, the signal level for rs485 Transmit Enable is the
identity, not the inversion:  If the transceiver expects a "high" RTS
signal for Transmit Enable, the signal coming out of the uart must also
be high, so TIOCM_RTS must be *cleared* in port->mctrl.

The commit did the exact opposite, but it's easy to see why given the
confusing semantics of rs232 and rs485.  Fix it.

Hi Lukas,

unfortunately this commit, which has been backported to v5.4.x, seems
to break RS485 functionality on our iMX boards.

In my opinion, we have a correct dts setup: we're using a GPIO to
control the RS485 transceiver Driver Enable pin, which is specified as
active high. The iMX GPIO pin directly drives the tranceiver DE pin.

Therefore, we did not specify rs485-rts-active-low, so according to the
dts-binding documentation RTS should be driven high when sending
(default). Also the GPIO is registered as GPIO_ACTIVE_HIGH at rts-
gpios.

	&uart2 {
	        pinctrl-names = "default";
	
	        pinctrl-0 = <&pinctrl_uart2_rs485>;
	        rts-gpios = <&gpio1 23 GPIO_ACTIVE_HIGH>;
	        linux,rs485-enabled-at-boot-time;
	        status = "okay";
	};

So, I'm unsure now whether your commit is wrong, or whether there was a
workaround in the iMX driver which now needs to be undone, or whether I
made a wrong assumption at the dts setup.

Could you please share your opinion on this.

Thanks,
Henri


Fixes: a6845e1e1b78 ("serial: core: Consider rs485 settings to drive
RTS")
Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx # v4.14+
Cc: Rafael Gago Castano <rgc@xxxxxx>
Cc: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
Cc: Su Bao Cheng <baocheng.su@xxxxxxxxxxx>
---
 drivers/tty/serial/serial_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c
b/drivers/tty/serial/serial_core.c
index 29f4781..259f28e 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -162,7 +162,7 @@ static void uart_port_dtr_rts(struct uart_port
*uport, int raise)
        int RTS_after_send = !!(uport->rs485.flags &
SER_RS485_RTS_AFTER_SEND);
 
        if (raise) {
-               if (rs485_on && !RTS_after_send) {
+               if (rs485_on && RTS_after_send) {
                        uart_set_mctrl(uport, TIOCM_DTR);
                        uart_clear_mctrl(uport, TIOCM_RTS);
                } else {
@@ -171,7 +171,7 @@ static void uart_port_dtr_rts(struct uart_port
*uport, int raise)
        } else {
                unsigned int clear = TIOCM_DTR;
 
-               clear |= (!rs485_on || !RTS_after_send) ? TIOCM_RTS :
0;
+               clear |= (!rs485_on || RTS_after_send) ? TIOCM_RTS : 0;
                uart_clear_mctrl(uport, clear);
        }
 }

Attachment: smime.p7s
Description: S/MIME cryptographic signature


[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux