serial: 8250_of: set UART_CAP_NOTEMT for rts-gpios RS485 direction
PATCH 1/1 serial: 8250_of: set UART_CAP_NOTEMT for rts-gpios RS485 direction
__stop_tx() (8250_port.c) only calls the RS485 rs485_stop_tx() hook (which de-asserts the direction GPIO/RTS line) once it has observed both UART_LSR_THRE and UART_LSR_TEMT for the last byte. If TEMT is never seen and the driver hasn't set UART_CAP_NOTEMT, the function returns without scheduling any retry -- the direction line is left asserted (driver enabled) forever, with nothing to un-stick it short of another kernel-visible LSR event.
of_platform_serial_setup() unconditionally wires up the generic em485 GPIO-RTS RS485 support (rs485_config/rs485_start_tx/rs485_stop_tx) for every port it registers, but never sets UART_CAP_NOTEMT, so any board using this driver whose 16550-compatible core doesn't reliably surface TEMT for its shift register hits the stuck-direction-GPIO case above.
Confirmed live on an ath79 QCA9531 board (SoC-internal ns16550a- compatible UART, RS485 transceiver DE/RE tied together on a GPIO via rts-gpios, linux,rs485-enabled-at-boot-time): the direction GPIO correctly asserts for the duration of a transmit, but never de-asserts afterwards -- confirmed by sampling the GPIO's debugfs state through and after a multi-hundred-byte write, on both the first transmit and repeated back-to-back transmits. Setting UART_CAP_NOTEMT, which makes __stop_tx() fall back to a frame-time- based timer instead of waiting indefinitely on TEMT, makes the direction GPIO reliably return low right after each transmit completes.
Scope the fix to ports that declare a GPIO-controlled direction line (rts-gpios), rather than setting it unconditionally for every port this driver registers: this is the class of hardware actually affected (RTS state has to be explicitly un-stuck by software, unlike a UART's native RTS pin), and it avoids adding the extra frame-time margin to ports relying on the native RTS pin, which has not been observed to need it.
--- drivers/tty/serial/8250/8250_of.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
drivers/tty/serial/8250/8250_of.c +16 -0
@@ -156,6 +156,22 @@ static int of_platform_serial_setup(struct platform_device *ofdev, up->rs485_start_tx = serial8250_em485_start_tx; up->rs485_stop_tx = serial8250_em485_stop_tx; +/* +* This generic driver never enables a dedicated line-status +* interrupt on TEMT, so for ports whose RS485 direction is +* controlled via a GPIO (rts-gpios) rather than the native RTS +* pin, __stop_tx() (8250_port.c) can see THRE without TEMT on the +* last byte and bail out without ever retrying -- leaving the +* direction GPIO stuck asserted after the last byte sent, on +* hardware whose shift register doesn't reliably surface TEMT. +* UART_CAP_NOTEMT makes it fall back to a frame-time-based timer +* instead of waiting on that interrupt. Scoped to rts-gpios users +* only, to avoid changing timing for ports relying on the native +* RTS pin, which this has not been observed to affect. +*/ +if (of_property_present (np,"rts-gpios" )) + up->capabilities |=UART_CAP_NOTEMT ; +switch (type) {case PORT_RT2880 : ret =rt288x_setup (port);
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The newly added UART_CAP_NOTEMT flag is either silently overwritten or causes the port to lose all…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added UART_CAP_NOTEMT flag is either silently overwritten or causes the port to lose all of its default hardware capabilities. --
commit 1d8dc615b4080d6f9bde37f54eafa34f59649ede
Author: Nicolas Thibert <[email protected]>
serial: 8250_of: set UART_CAP_NOTEMT for rts-gpios RS485 direction
This patch sets UART_CAP_NOTEMT for RS485 ports using rts-gpios to prevent the direction GPIO from getting stuck due to missing TEMT interrupts.
Quoted 15 lines
drivers/tty/serial/8250/8250_of.c +7
-0@@ -212,6 +212,22 @@ static int of_platform_serial_setup(struct platform_device *ofdev, up->rs485_start_tx = serial8250_em485_start_tx; up->rs485_stop_tx = serial8250_em485_stop_tx; +/* +* This generic driver never enables a dedicated line-status +* interrupt on TEMT, so for ports whose RS485 direction is +* controlled via a GPIO (rts-gpios) rather than the native RTS +* pin, __stop_tx() (8250_port.c) can see THRE without TEMT on the +* last byte and bail out without ever retrying -- leaving the +* direction GPIO stuck asserted after the last byte sent, on
drivers/tty/serial/8250/8250_of.c static int of_platform_serial_setup(struct platform_device *ofdev, · line 231
Quoted 10 lines
drivers/tty/serial/8250/8250_of.c +9
-0+* hardware whose shift register doesn't reliably surface TEMT. +* UART_CAP_NOTEMT makes it fall back to a frame-time-based timer +* instead of waiting on that interrupt. Scoped to rts-gpios users +* only, to avoid changing timing for ports relying on the native +* RTS pin, which this has not been observed to affect. +*/ +if (of_property_present (np,"rts-gpios" )) + up->capabilities |=UART_CAP_NOTEMT ; +switch (type) {
[Severity: High]
If the device explicitly configures a FIFO size, does the caller overwrite this newly added capability?
When of_platform_serial_probe() calls of_platform_serial_setup(), it does this right after:
drivers/tty/serial/8250/8250_of.c:of_platform_serial_probe() {
...
ret = of_platform_serial_setup(ofdev, port_type, &port8250, info);
if (ret)
goto err_free;
if (port8250.port.fifosize)
port8250.capabilities = UART_CAP_FIFO;
...
}Will this direct assignment completely erase the UART_CAP_NOTEMT flag?
Conversely, if the device does not specify a fifosize, port8250.capabilities will be non-zero because it holds the UART_CAP_NOTEMT flag. Does this cause the port to miss all its default hardware capabilities?
Later during registration, serial8250_set_defaults() checks if capabilities is zero before assigning defaults:
drivers/tty/serial/8250/8250_port.c:serial8250_set_defaults() {
...
if (!up->capabilities)
up->capabilities = uart_config[port->type].flags;
...
}Since up->capabilities is non-zero, this assignment is skipped entirely, stripping the port of default features like UART_CAP_FIFO and UART_CAP_SLEEP.
Could we change the assignment in of_platform_serial_probe() to use |=, and find a different way to merge UART_CAP_NOTEMT with the default flags?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1