* [PATCH v4] serial: max310x: drive RTS in software when hardware delays are too short
@ 2026-09-16 7:10 Tapio Reijonen
2026-09-23 10:39 ` Greg Kroah-Hartman
2026-09-29 7:13 ` Tapio Reijonen
0 siblings, 2 replies; 3+ messages in thread
From: Tapio Reijonen @ 2026-09-16 7:10 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: linux-kernel, linux-serial, Hugo Villeneuve, Tapio Reijonen,
Tapio Reijonen
max310x_rs485_config() rejected delay_rts_before_send and
delay_rts_after_send values larger than 0x0f with -ERANGE, which made
the UART core wipe port->rs485 in uart_rs485_config() and silently
disable RS485. The HDPIXDELAY register holds the setup and hold
delays in 4-bit-per-direction bit-times, so even values inside that
range only encode a fraction of a millisecond at typical baud rates
and the chip's hardware auto-RTS path cannot cover the millisecond
range the kernel UART layer expresses.
Add a software-driven RTS path that takes over whenever the hardware
cannot represent the requested timing:
* Cache the current baud rate and the per-character on-the-wire
duration in max310x_set_termios() so the decision below can use
them.
* max310x_set_rts_ctl_params() picks software or hardware timing:
- software if delay_rts_before_send or delay_rts_after_send in
milliseconds exceeds what 15 bit-times can encode at the
current baud, or if the requested RTS polarity cannot be
produced by the chip's auto-RTS engine;
- hardware otherwise, converting the millisecond delays to
bit-times (rounded up, capped at 15) and programming
MODE1.TRNSCVCTRL plus IRDA.RTSINVERT to drive RTS with the
requested polarity. RTS is left deasserted at idle; the chip's
auto-RTS engine owns the transceiver during transmission.
* When software timing is selected the RTS envelope is driven by a
single hrtimer, re-used for the before- and after-send phases (the
phase is tracked in tx_state), plus a single rts_work that toggles
RTS. max310x_start_tx() queues rts_work to assert RTS; rts_work
arms the timer for the before-send delay only after the RTS edge is
on the wire, so data is never shifted before RTS is asserted. The
timer expiry kicks tx_work to fill the chip FIFO; once that FIFO is
empty (max310x_handle_tx()) the same timer is re-armed for one
character duration plus the after-send delay, after which rts_work
releases RTS. The hold is armed only while tx_state is
MAX310X_TX_SEND, re-checked under port->lock because
max310x_handle_tx() runs from a worker that does not hold it and the
port can be shut down in the meantime. Using one timer and one
rts_work keeps the before-
and after-send phases mutually exclusive and the RTS toggles
ordered, which matters for back-to-back writes and on SMP.
* The LCR register carries the TX break and RTS bits next to the
termios bits, so max310x_set_termios() updates only the bits it owns
instead of writing the register absolutely, and
max310x_set_rts_ctl_params() settles RTS to the idle level only
while tx_state is MAX310X_TX_OFF. Both can run while a software
timed envelope is in flight - serial_core calls ->set_termios()
without port->lock, and max310x_rs485_config() schedules a
reconfigure on every TIOCSRS485 - and would otherwise release the
transceiver mid-character.
* max310x_shutdown() waits for transmission to finish before powering
the port down, so close() cannot truncate the final byte.
On the software path it waits out any in-flight RTS envelope
(bounded) so the last character and its after-send hold complete.
On the hardware path it lets the chip FIFO drain and the last
character clock out of the shift register: tx_empty only reports
the TX FIFO empty, not the shift register, so without this the
port could be powered down mid-character. The delay timer and
rts_work are then cancelled unconditionally: the software/hardware
decision is recomputed on every reconfigure, so a TIOCSRS485 can
clear sw_rts_during_tx while an envelope is still in flight, and
neither may outlive the port. That flag is published in a single
store for the same reason.
* max310x_rs485_config() now clamps the delays to the UART core's
RS485_MAX_RTS_DELAY (100 ms) instead of rejecting them, and
cancels the pending delay timer when RS485 is disabled.
* max310x_break_ctl() drives RTS manually for the break duration. The
chip's auto-RTS only asserts the transceiver while FIFO data is
shifting out, and a break is not FIFO data, so on the hardware path
it also disables auto-RTS for the break and restores it when the
break ends. The software path drives the configured RS485 RTS
polarity; on the hardware path IRDA.RTSINVERT already inverts the
RTS_ output stage, so break_state is driven as it is.
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
Changes in v4:
- max310x_shutdown() cancels the delay timer and rts_work
unconditionally. The software/hardware decision is recomputed on every
reconfigure, so a TIOCSRS485 can clear sw_rts_during_tx while an
envelope is still in flight; gating the cancellation on that flag left
the timer armed against a port that was about to be powered down.
- max310x_delayed_stop_tx() re-checks under port->lock and arms the
after-send hold only while tx_state is MAX310X_TX_SEND. It runs from a
worker that does not hold the lock, and it cleared cancel_tx_delay_tmr
unconditionally and tested tx_state != MAX310X_TX_WAIT_BEFORE_SEND,
which is also true for MAX310X_TX_OFF, so it could resurrect an
envelope that shutdown() had just cancelled.
- sw_rts_during_tx is computed into a local and published in a single
store, and read with READ_ONCE(). It was assigned false and only then
recomputed, so any concurrent reader could observe a transient false.
- Triage of the review findings that prompted the above:
https://lore.kernel.org/r/c15330e4-25e0-4c50-9f48-dbb8d51f5823@vaisala.com
- Link to v3: https://lore.kernel.org/r/20260915-max310x-rs485-sw-delay-v3-1-7d20a4a4ab52@vaisala.com
Changes in v3:
- max310x_set_termios() updates only the termios-owned LCR bits instead
of writing LCR absolutely. TX break and the software-timed RS485 RTS
live in the same register, and serial_core calls ->set_termios()
without port->lock, so a tcsetattr() concurrent with an in-flight
envelope released the transceiver mid-character.
- max310x_set_rts_ctl_params() only settles RTS to the idle level while
tx_state is MAX310X_TX_OFF. It runs from rs_work, which
max310x_rs485_config() schedules on every TIOCSRS485, and forcing the
idle level there unconditionally had the same mid-character effect.
- max310x_break_ctl() applies the configured RTS polarity on the
software path; a port with rs485-rts-active-low drove the wrong level
during a break. The hardware path keeps driving break_state as it is:
IRDA.RTSINVERT inverts the RTS_ output stage itself (MAX14830
datasheet, IrDA register bit 2), so both polarities already come out
right there.
- Add max310x_rts_level() for the flag-to-level selection, which is now
needed at four sites.
- Use uart_port_unlock()/uart_port_lock() at the two sites that drop
port->lock around hrtimer_cancel().
- Not done, although I said I would: skipping the before-send re-arm
when tx_state is already MAX310X_TX_SEND. Returning early there also
removes the interlock that keeps max310x_delayed_stop_tx() from arming
the after-send hold while a fresh write is still queued, which can
release RTS mid-character. The txlvl == 0 gate in
max310x_delayed_stop_tx() already keeps the timing correct; the cost
of leaving it alone is one spurious before-send delay mid-stream.
- Review that prompted the above, on the v2 resend:
https://lore.kernel.org/r/20260914-max310x-rs485-sw-delay-v2-1-9a7b681e64ce@vaisala.com
- Link to v2: https://lore.kernel.org/r/20260811-max310x-rs485-sw-delay-v2-1-e34283205789@vaisala.com
Changes in v2:
- Use tty_get_frame_size() instead of open-coding the frame size
(Jiri Slaby). The helper was left with a single expression and one
caller, so it is dropped and the calculation now lives in
max310x_set_termios(). tty_get_frame_size() additionally accounts for
ADDRB, which the open-coded version ignored.
- Use guard(spinlock_irqsave) instead of spin_lock_irqsave() and
spin_unlock_irqrestore() (Jiri Slaby), in max310x_rts_work_proc(),
max310x_tmr_tx() and max310x_delayed_stop_tx(). The bare
spin_lock()/spin_unlock() pairs in max310x_start_tx() and
max310x_rs485_config() are left alone: both are called with port->lock
held by the caller and only drop it around hrtimer_cancel(), which
guard() cannot express.
- Link to v1: https://lore.kernel.org/r/20260709-max310x-rs485-sw-delay-v1-1-454ac10b937a@vaisala.com
---
drivers/tty/serial/max310x.c | 408 +++++++++++++++++++++++++++++++++++++++----
1 file changed, 372 insertions(+), 36 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 022502986c5fcf1ff4de9328746ddc71677be730..c217857f5d32a85728f82ad52e02c3a9075eb0da 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -15,6 +15,7 @@
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/gpio/driver.h>
+#include <linux/hrtimer.h>
#include <linux/i2c.h>
#include <linux/kconfig.h>
#include <linux/module.h>
@@ -158,10 +159,17 @@
#define MAX310X_LCR_FORCEPARITY_BIT (1 << 5) /* 9-bit multidrop parity */
#define MAX310X_LCR_TXBREAK_BIT (1 << 6) /* TX break enable */
#define MAX310X_LCR_RTS_BIT (1 << 7) /* RTS pin control */
+/* LCR bits owned by termios; TX break and RTS are driven elsewhere */
+#define MAX310X_LCR_TERMIOS_MASK GENMASK(5, 0)
/* IRDA register bits */
#define MAX310X_IRDA_IRDAEN_BIT (1 << 0) /* IRDA mode enable */
#define MAX310X_IRDA_SIR_BIT (1 << 1) /* SIR mode enable */
+#define MAX310X_IRDA_RTSINVERT_BIT (1 << 2) /* Invert RTS output */
+
+/* HDPIXDELAY accessor macros */
+#define MAX310X_HDPIXDELAY_SETUP(val) (((val) & 0x0f) << 4)
+#define MAX310X_HDPIXDELAY_HOLD(val) ((val) & 0x0f)
/* Flow control trigger level register masks */
#define MAX310X_FLOWLVL_HALT_MASK GENMASK(3, 0) /* Flow control halt level */
@@ -290,12 +298,27 @@ struct max310x_devtype {
u8 power_bit; /* Bit for sleep or power-off mode (active high). */
};
+/* Software-timed RS485 RTS envelope phase */
+enum max310x_tx_state {
+ MAX310X_TX_OFF, /* idle, RTS released */
+ MAX310X_TX_WAIT_BEFORE_SEND, /* RTS asserted, before-send delay */
+ MAX310X_TX_SEND, /* data in flight, awaiting TX-empty */
+ MAX310X_TX_WAIT_AFTER_SEND, /* data drained, after-send hold */
+};
+
struct max310x_one {
struct uart_port port;
struct work_struct tx_work;
struct work_struct md_work;
struct work_struct rs_work;
+ struct work_struct rts_work;
+ struct hrtimer tx_delay_tmr;
struct regmap *regmap;
+ ktime_t one_character_duration;
+ unsigned int baud;
+ bool sw_rts_during_tx;
+ bool cancel_tx_delay_tmr;
+ enum max310x_tx_state tx_state;
u8 rx_buf[MAX310X_FIFO_SIZE];
};
@@ -680,6 +703,42 @@ static void max310x_batch_read(struct uart_port *port, u8 *rxbuf, unsigned int l
regmap_noinc_read(one->regmap, MAX310X_RHR_REG, rxbuf, len);
}
+static void max310x_rts_ctl(struct uart_port *port, bool rts_state)
+{
+ max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_RTS_BIT,
+ rts_state ? MAX310X_LCR_RTS_BIT : 0);
+}
+
+/* RTS level for the transmitting or the idle phase of an RS485 envelope */
+static bool max310x_rts_level(struct uart_port *port, bool active)
+{
+ return active ? (port->rs485.flags & SER_RS485_RTS_ON_SEND) :
+ (port->rs485.flags & SER_RS485_RTS_AFTER_SEND);
+}
+
+/*
+ * Drive the RS485 RTS line to match the current tx_state. This is the only
+ * place that touches RTS, and it reads tx_state rather than a fixed
+ * assert/deassert intent, so a newer assert is never clobbered by a stale
+ * release. It also arms the before-send timer once the RTS edge is on the wire,
+ * so data is never shifted before RTS is asserted.
+ */
+static void max310x_rts_work_proc(struct work_struct *ws)
+{
+ struct max310x_one *one = container_of(ws, struct max310x_one, rts_work);
+ struct uart_port *port = &one->port;
+ bool rts_on = READ_ONCE(one->tx_state) != MAX310X_TX_OFF;
+
+ max310x_rts_ctl(port, max310x_rts_level(port, rts_on));
+
+ guard(spinlock_irqsave)(&port->lock);
+ if (READ_ONCE(one->tx_state) == MAX310X_TX_WAIT_BEFORE_SEND &&
+ !one->cancel_tx_delay_tmr && !hrtimer_active(&one->tx_delay_tmr))
+ hrtimer_start(&one->tx_delay_tmr,
+ ms_to_ktime(port->rs485.delay_rts_before_send),
+ HRTIMER_MODE_REL);
+}
+
static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
{
struct max310x_one *one = to_max310x_port(port);
@@ -776,6 +835,67 @@ static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
tty_flip_buffer_push(&port->state->port);
}
+static enum hrtimer_restart max310x_tmr_tx(struct hrtimer *timer)
+{
+ struct max310x_one *one = container_of(timer, struct max310x_one,
+ tx_delay_tmr);
+
+ guard(spinlock_irqsave)(&one->port.lock);
+ if (!one->cancel_tx_delay_tmr) {
+ if (READ_ONCE(one->tx_state) == MAX310X_TX_WAIT_AFTER_SEND) {
+ /* After-send hold elapsed: drop RTS via the rts worker. */
+ WRITE_ONCE(one->tx_state, MAX310X_TX_OFF);
+ schedule_work(&one->rts_work);
+ } else {
+ WRITE_ONCE(one->tx_state, MAX310X_TX_SEND);
+ schedule_work(&one->tx_work);
+ }
+ }
+
+ return HRTIMER_NORESTART;
+}
+
+static void max310x_delayed_stop_tx(struct uart_port *port)
+{
+ struct max310x_one *one = to_max310x_port(port);
+ unsigned int txlvl;
+
+ if (READ_ONCE(one->tx_state) == MAX310X_TX_OFF)
+ return;
+
+ /*
+ * The kfifo can be empty while the chip TX FIFO is still draining, so arm
+ * the after-send hold only once the chip FIFO is empty too - the TX-empty
+ * interrupt re-invokes us then. Otherwise the hold starts early and RTS
+ * drops mid-character, clipping the last byte(s).
+ */
+ txlvl = max310x_port_read(port, MAX310X_TXFIFOLVL_REG);
+ if (txlvl)
+ return;
+
+ /*
+ * Runs from tx_work without port->lock, so re-check the state under it:
+ * shutdown() may have cancelled the envelope meanwhile. Only
+ * MAX310X_TX_SEND may arm the hold.
+ */
+ guard(spinlock_irqsave)(&one->port.lock);
+ if (one->cancel_tx_delay_tmr ||
+ READ_ONCE(one->tx_state) != MAX310X_TX_SEND)
+ return;
+
+ if (!hrtimer_active(&one->tx_delay_tmr)) {
+ /*
+ * Add one character for the byte still in the shift register -
+ * TX-empty fires as it enters, not as it leaves.
+ */
+ ktime_t delay = ktime_add_ms(one->one_character_duration,
+ port->rs485.delay_rts_after_send);
+
+ WRITE_ONCE(one->tx_state, MAX310X_TX_WAIT_AFTER_SEND);
+ hrtimer_start(&one->tx_delay_tmr, delay, HRTIMER_MODE_REL);
+ }
+}
+
static void max310x_handle_tx(struct uart_port *port)
{
struct tty_port *tport = &port->state->port;
@@ -787,8 +907,13 @@ static void max310x_handle_tx(struct uart_port *port)
return;
}
- if (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(port))
+ if (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(port)) {
+ struct max310x_one *one = to_max310x_port(port);
+
+ if (READ_ONCE(one->sw_rts_during_tx))
+ max310x_delayed_stop_tx(port);
return;
+ }
/*
* It's a circ buffer -- wrap around.
@@ -813,11 +938,48 @@ static void max310x_handle_tx(struct uart_port *port)
uart_write_wakeup(port);
}
+/*
+ * Begin a software-timed RTS envelope: set the before-send phase and queue the
+ * rts worker to assert RTS. tx_state is set synchronously here (start_tx() holds
+ * port.lock) so close()/shutdown can see an envelope is in flight; rts_work then
+ * asserts RTS and arms the before-send timer (see there).
+ */
+static void max310x_delayed_start_tx(struct uart_port *port)
+{
+ struct max310x_one *one = to_max310x_port(port);
+
+ WRITE_ONCE(one->tx_state, MAX310X_TX_WAIT_BEFORE_SEND);
+ one->cancel_tx_delay_tmr = false;
+ schedule_work(&one->rts_work);
+}
+
+/* called with port.lock taken and irqs off */
static void max310x_start_tx(struct uart_port *port)
{
struct max310x_one *one = to_max310x_port(port);
- schedule_work(&one->tx_work);
+ if (READ_ONCE(one->sw_rts_during_tx)) {
+ /*
+ * The before- and after-send phases share one delay timer. If an
+ * after-send release is pending, cancel it before starting a new
+ * TX so the just-asserted RTS is not yanked; re-arming the timer
+ * for the before-send phase then supersedes the release.
+ */
+ int res = 0;
+
+ if (READ_ONCE(one->tx_state) == MAX310X_TX_WAIT_AFTER_SEND)
+ res = hrtimer_try_to_cancel(&one->tx_delay_tmr);
+ if (unlikely(res == -1)) {
+ one->cancel_tx_delay_tmr = true;
+ uart_port_unlock(port);
+ hrtimer_cancel(&one->tx_delay_tmr);
+ uart_port_lock(port);
+ }
+
+ max310x_delayed_start_tx(port);
+ } else {
+ schedule_work(&one->tx_work);
+ }
}
static irqreturn_t max310x_port_irq(struct max310x_port *s, int portno)
@@ -843,7 +1005,7 @@ static irqreturn_t max310x_port_irq(struct max310x_port *s, int portno)
if (rxlen)
max310x_handle_rx(port, rxlen);
if (ists & MAX310X_IRQ_TXEMPTY_BIT)
- max310x_start_tx(port);
+ schedule_work(&s->p[portno].tx_work);
} while (1);
return res;
@@ -927,15 +1089,113 @@ static void max310x_set_mctrl(struct uart_port *port, unsigned int mctrl)
static void max310x_break_ctl(struct uart_port *port, int break_state)
{
+ struct max310x_one *one = to_max310x_port(port);
+
max310x_port_update(port, MAX310X_LCR_REG,
MAX310X_LCR_TXBREAK_BIT,
break_state ? MAX310X_LCR_TXBREAK_BIT : 0);
+
+ if (!(port->rs485.flags & SER_RS485_ENABLED))
+ return;
+
+ /*
+ * Drive RTS manually for the break duration. HW auto-RTS only asserts
+ * the transceiver while FIFO data is shifting out, and a break is not
+ * FIFO data, so on the HW path also disable auto-RTS for the break and
+ * restore it when the break ends. There IRDA.RTSINVERT already inverts
+ * the pin for an active-low RTS, so break_state is driven as it is;
+ * the software path applies the polarity itself.
+ */
+ if (READ_ONCE(one->sw_rts_during_tx)) {
+ max310x_rts_ctl(port, max310x_rts_level(port, break_state));
+ } else {
+ max310x_port_update(port, MAX310X_MODE1_REG,
+ MAX310X_MODE1_TRNSCVCTRL_BIT,
+ break_state ? 0 : MAX310X_MODE1_TRNSCVCTRL_BIT);
+ max310x_rts_ctl(port, break_state);
+ }
+}
+
+/*
+ * Pick hardware or software RTS timing for the current port. The chip can
+ * deliver up to 15 bit-times of setup/hold delay via HDPIXDELAY; anything
+ * longer (or any RTS polarity the chip cannot produce automatically) must
+ * be driven by software via tx_delay_tmr and rts_work.
+ */
+static void max310x_set_rts_ctl_params(struct max310x_one *one)
+{
+ const unsigned int max_bit_dly = 15;
+ struct uart_port *port = &one->port;
+ unsigned long max_hw_delay_ns = 0;
+ unsigned int setup = 0, hold = 0;
+ u8 mode1 = 0, irda = 0;
+ bool sw_rts = false;
+
+ if (!(port->rs485.flags & SER_RS485_ENABLED))
+ goto out;
+
+ if (one->baud)
+ max_hw_delay_ns = NSEC_PER_SEC / one->baud * max_bit_dly;
+
+ if ((u64)port->rs485.delay_rts_before_send * NSEC_PER_MSEC > max_hw_delay_ns ||
+ (u64)port->rs485.delay_rts_after_send * NSEC_PER_MSEC > max_hw_delay_ns ||
+ !!(port->rs485.flags & SER_RS485_RTS_ON_SEND) ==
+ !!(port->rs485.flags & SER_RS485_RTS_AFTER_SEND))
+ sw_rts = true;
+
+ if (sw_rts) {
+ setup = 0;
+ hold = 0;
+ goto out;
+ }
+
+ /* Convert milliseconds to bit-times, rounding up. */
+ setup = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_before_send,
+ MSEC_PER_SEC);
+ hold = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_after_send,
+ MSEC_PER_SEC);
+ setup = min(setup, max_bit_dly);
+ hold = min(hold, max_bit_dly);
+
+out:
+ /* Assign once; a transient false would be seen by other readers. */
+ WRITE_ONCE(one->sw_rts_during_tx, sw_rts);
+
+ max310x_port_write(port, MAX310X_HDPIXDELAY_REG,
+ MAX310X_HDPIXDELAY_SETUP(setup) |
+ MAX310X_HDPIXDELAY_HOLD(hold));
+
+ if (port->rs485.flags & SER_RS485_ENABLED) {
+ if (sw_rts) {
+ /*
+ * Only settle RTS at idle when no transmission owns it.
+ * A reconfigure while one is in flight - rs_work runs
+ * on every TIOCSRS485 - would otherwise release the
+ * transceiver mid-character.
+ */
+ if (READ_ONCE(one->tx_state) == MAX310X_TX_OFF)
+ max310x_rts_ctl(port,
+ max310x_rts_level(port, false));
+ } else {
+ mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
+ if (!(port->rs485.flags & SER_RS485_RTS_ON_SEND))
+ irda = MAX310X_IRDA_RTSINVERT_BIT;
+ }
+ } else {
+ max310x_rts_ctl(port, 0);
+ }
+
+ max310x_port_update(port, MAX310X_MODE1_REG,
+ MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
+ max310x_port_update(port, MAX310X_IRDA_REG,
+ MAX310X_IRDA_RTSINVERT_BIT, irda);
}
static void max310x_set_termios(struct uart_port *port,
struct ktermios *termios,
const struct ktermios *old)
{
+ unsigned int frame_bits = tty_get_frame_size(termios->c_cflag);
unsigned int lcr = 0, flow = 0;
int baud;
@@ -969,8 +1229,12 @@ static void max310x_set_termios(struct uart_port *port,
if (termios->c_cflag & CSTOPB)
lcr |= MAX310X_LCR_STOPLEN_BIT; /* 2 stops */
- /* Update LCR register */
- max310x_port_write(port, MAX310X_LCR_REG, lcr);
+ /*
+ * Update LCR register. Leave the TX break and RTS bits alone: they are
+ * driven by break_ctl() and by the software-timed RS485 RTS, and an
+ * absolute write here would release the transceiver mid-character.
+ */
+ max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_TERMIOS_MASK, lcr);
/* Set read status mask */
port->read_status_mask = MAX310X_LSR_RXOVR_BIT;
@@ -1042,38 +1306,55 @@ static void max310x_set_termios(struct uart_port *port,
/* Update timeout according to new baud rate */
uart_update_timeout(port, termios->c_cflag, baud);
+
+ /*
+ * Cache the new baud rate and the time it takes to clock out one
+ * character so the RTS-timing decision in max310x_set_rts_ctl_params()
+ * and the post-TX delay in max310x_delayed_stop_tx() can use them.
+ */
+ to_max310x_port(port)->baud = baud;
+ to_max310x_port(port)->one_character_duration =
+ us_to_ktime(DIV_ROUND_UP(USEC_PER_SEC * frame_bits, baud));
+ max310x_set_rts_ctl_params(to_max310x_port(port));
}
static void max310x_rs_proc(struct work_struct *ws)
{
struct max310x_one *one = container_of(ws, struct max310x_one, rs_work);
- unsigned int delay, mode1 = 0, mode2 = 0;
+ unsigned int mode2 = 0;
- delay = (one->port.rs485.delay_rts_before_send << 4) |
- one->port.rs485.delay_rts_after_send;
- max310x_port_write(&one->port, MAX310X_HDPIXDELAY_REG, delay);
+ max310x_set_rts_ctl_params(one);
- if (one->port.rs485.flags & SER_RS485_ENABLED) {
- mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
+ if (one->port.rs485.flags & SER_RS485_ENABLED &&
+ !(one->port.rs485.flags & SER_RS485_RX_DURING_TX))
+ mode2 = MAX310X_MODE2_ECHOSUPR_BIT;
- if (!(one->port.rs485.flags & SER_RS485_RX_DURING_TX))
- mode2 = MAX310X_MODE2_ECHOSUPR_BIT;
- }
-
- max310x_port_update(&one->port, MAX310X_MODE1_REG,
- MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
max310x_port_update(&one->port, MAX310X_MODE2_REG,
- MAX310X_MODE2_ECHOSUPR_BIT, mode2);
+ MAX310X_MODE2_ECHOSUPR_BIT, mode2);
}
+/* called with port.lock taken and irqs off */
static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios,
struct serial_rs485 *rs485)
{
struct max310x_one *one = to_max310x_port(port);
- if ((rs485->delay_rts_before_send > 0x0f) ||
- (rs485->delay_rts_after_send > 0x0f))
- return -ERANGE;
+ rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
+ rs485->delay_rts_after_send = min(rs485->delay_rts_after_send, 100U);
+
+ /*
+ * Make sure no SW-timed RTS toggle survives an RS485 disable, even
+ * if the delay timer happens to be running right now.
+ */
+ if (!(rs485->flags & SER_RS485_ENABLED)) {
+ one->cancel_tx_delay_tmr = true;
+ if (hrtimer_try_to_cancel(&one->tx_delay_tmr) == -1) {
+ uart_port_unlock(port);
+ hrtimer_cancel(&one->tx_delay_tmr);
+ uart_port_lock(port);
+ }
+ WRITE_ONCE(one->tx_state, MAX310X_TX_OFF);
+ }
port->rs485 = *rs485;
@@ -1084,6 +1365,7 @@ static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios
static int max310x_startup(struct uart_port *port)
{
+ struct max310x_one *one = to_max310x_port(port);
unsigned int val;
max310x_power(port, 1);
@@ -1098,21 +1380,20 @@ static int max310x_startup(struct uart_port *port)
max310x_port_update(port, MAX310X_MODE2_REG,
MAX310X_MODE2_FIFORST_BIT, 0);
- /* Configure mode1/mode2 to have rs485/rs232 enabled at startup */
- val = (clamp(port->rs485.delay_rts_before_send, 0U, 15U) << 4) |
- clamp(port->rs485.delay_rts_after_send, 0U, 15U);
- max310x_port_write(port, MAX310X_HDPIXDELAY_REG, val);
-
- if (port->rs485.flags & SER_RS485_ENABLED) {
- max310x_port_update(port, MAX310X_MODE1_REG,
- MAX310X_MODE1_TRNSCVCTRL_BIT,
- MAX310X_MODE1_TRNSCVCTRL_BIT);
+ /*
+ * Configure RTS timing (HW auto-RTS vs software-driven) and the
+ * RS485/RS232 mode bits. Don't hardcode HW auto-RTS here - let
+ * max310x_set_rts_ctl_params() pick HW or SW per the configured
+ * delays, otherwise the chip's auto-RTS would override the
+ * software RTS hold and the after-send delay is lost.
+ */
+ max310x_set_rts_ctl_params(one);
- if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
- max310x_port_update(port, MAX310X_MODE2_REG,
- MAX310X_MODE2_ECHOSUPR_BIT,
- MAX310X_MODE2_ECHOSUPR_BIT);
- }
+ if (port->rs485.flags & SER_RS485_ENABLED &&
+ !(port->rs485.flags & SER_RS485_RX_DURING_TX))
+ max310x_port_update(port, MAX310X_MODE2_REG,
+ MAX310X_MODE2_ECHOSUPR_BIT,
+ MAX310X_MODE2_ECHOSUPR_BIT);
/*
* Configure flow control levels:
@@ -1134,9 +1415,56 @@ static int max310x_startup(struct uart_port *port)
static void max310x_shutdown(struct uart_port *port)
{
+ struct max310x_one *one = to_max310x_port(port);
+
+ /*
+ * Drain any in-flight software-timed RTS envelope before the port is
+ * powered down, so the last character and its after-send hold complete
+ * - close() can reach shutdown with data still queued and a before-send
+ * delay pending. The loop ends when the envelope does (tx_state == OFF);
+ * the bound is just a worst-case safety cap.
+ */
+ if (READ_ONCE(one->sw_rts_during_tx)) {
+ unsigned int loops = port->rs485.delay_rts_before_send +
+ port->rs485.delay_rts_after_send +
+ DIV_ROUND_UP_ULL((kfifo_len(&port->state->port.xmit_fifo) +
+ port->fifosize) *
+ ktime_to_us(one->one_character_duration),
+ USEC_PER_MSEC);
+
+ while (READ_ONCE(one->tx_state) != MAX310X_TX_OFF && loops-- > 0)
+ fsleep(USEC_PER_MSEC);
+ } else {
+ /*
+ * HW auto-RTS path: the tty layer waits for tx_empty before
+ * close(), but tx_empty only reflects the chip TX FIFO - the
+ * last character may still be in the transmit shift register.
+ * Let the FIFO drain and the final character clock out before
+ * the port is powered down, otherwise close() truncates the last
+ * byte on the wire as the chip auto-RTS turnaround clips it.
+ */
+ unsigned int loops = port->fifosize + 1;
+
+ while (!max310x_tx_empty(port) && loops-- > 0)
+ fsleep(ktime_to_us(one->one_character_duration));
+ fsleep(ktime_to_us(one->one_character_duration));
+ }
+
+ /*
+ * Cancel unconditionally: sw_rts_during_tx can have turned false while
+ * an envelope was in flight, and neither may outlive the port.
+ */
+ one->cancel_tx_delay_tmr = true;
+ hrtimer_cancel(&one->tx_delay_tmr);
+ cancel_work_sync(&one->rts_work);
+ WRITE_ONCE(one->tx_state, MAX310X_TX_OFF);
+
/* Disable all interrupts */
max310x_port_write(port, MAX310X_IRQEN_REG, 0);
+ if (READ_ONCE(one->sw_rts_during_tx))
+ max310x_rts_ctl(port, max310x_rts_level(port, false));
+
max310x_power(port, 0);
}
@@ -1291,7 +1619,8 @@ static int max310x_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
#endif
static const struct serial_rs485 max310x_rs485_supported = {
- .flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RX_DURING_TX,
+ .flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
+ SER_RS485_RTS_AFTER_SEND | SER_RS485_RX_DURING_TX,
.delay_rts_before_send = 1,
.delay_rts_after_send = 1,
};
@@ -1425,6 +1754,11 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
INIT_WORK(&s->p[i].md_work, max310x_md_proc);
/* Initialize queue for changing RS485 mode */
INIT_WORK(&s->p[i].rs_work, max310x_rs_proc);
+ /* Initialize queue for software-driven RTS toggling */
+ INIT_WORK(&s->p[i].rts_work, max310x_rts_work_proc);
+ hrtimer_setup(&s->p[i].tx_delay_tmr, max310x_tmr_tx,
+ CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+ s->p[i].tx_state = MAX310X_TX_OFF;
}
#ifdef CONFIG_GPIOLIB
@@ -1535,6 +1869,8 @@ static void max310x_remove(struct device *dev)
int i;
for (i = 0; i < s->devtype->nr; i++) {
+ hrtimer_cancel(&s->p[i].tx_delay_tmr);
+ cancel_work_sync(&s->p[i].rts_work);
cancel_work_sync(&s->p[i].tx_work);
cancel_work_sync(&s->p[i].md_work);
cancel_work_sync(&s->p[i].rs_work);
---
base-commit: 9505146e885b1a842118aa6410f737290c4a5a32
change-id: 20260513-max310x-rs485-sw-delay-a306d783d529
Best regards,
--
Tapio Reijonen <tapio.reijonen@vaisala.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v4] serial: max310x: drive RTS in software when hardware delays are too short
2026-09-16 7:10 [PATCH v4] serial: max310x: drive RTS in software when hardware delays are too short Tapio Reijonen
@ 2026-09-23 10:39 ` Greg Kroah-Hartman
2026-09-29 7:13 ` Tapio Reijonen
1 sibling, 0 replies; 3+ messages in thread
From: Greg Kroah-Hartman @ 2026-09-23 10:39 UTC (permalink / raw)
To: Tapio Reijonen
Cc: Jiri Slaby, linux-kernel, linux-serial, Hugo Villeneuve, Tapio Reijonen
On Wed, Sep 16, 2026 at 07:10:54AM +0000, Tapio Reijonen wrote:
> max310x_rs485_config() rejected delay_rts_before_send and
> delay_rts_after_send values larger than 0x0f with -ERANGE, which made
> the UART core wipe port->rs485 in uart_rs485_config() and silently
> disable RS485. The HDPIXDELAY register holds the setup and hold
> delays in 4-bit-per-direction bit-times, so even values inside that
> range only encode a fraction of a millisecond at typical baud rates
> and the chip's hardware auto-RTS path cannot cover the millisecond
> range the kernel UART layer expresses.
>
> Add a software-driven RTS path that takes over whenever the hardware
> cannot represent the requested timing:
>
> * Cache the current baud rate and the per-character on-the-wire
> duration in max310x_set_termios() so the decision below can use
> them.
> * max310x_set_rts_ctl_params() picks software or hardware timing:
> - software if delay_rts_before_send or delay_rts_after_send in
> milliseconds exceeds what 15 bit-times can encode at the
> current baud, or if the requested RTS polarity cannot be
> produced by the chip's auto-RTS engine;
> - hardware otherwise, converting the millisecond delays to
> bit-times (rounded up, capped at 15) and programming
> MODE1.TRNSCVCTRL plus IRDA.RTSINVERT to drive RTS with the
> requested polarity. RTS is left deasserted at idle; the chip's
> auto-RTS engine owns the transceiver during transmission.
> * When software timing is selected the RTS envelope is driven by a
> single hrtimer, re-used for the before- and after-send phases (the
> phase is tracked in tx_state), plus a single rts_work that toggles
> RTS. max310x_start_tx() queues rts_work to assert RTS; rts_work
> arms the timer for the before-send delay only after the RTS edge is
> on the wire, so data is never shifted before RTS is asserted. The
> timer expiry kicks tx_work to fill the chip FIFO; once that FIFO is
> empty (max310x_handle_tx()) the same timer is re-armed for one
> character duration plus the after-send delay, after which rts_work
> releases RTS. The hold is armed only while tx_state is
> MAX310X_TX_SEND, re-checked under port->lock because
> max310x_handle_tx() runs from a worker that does not hold it and the
> port can be shut down in the meantime. Using one timer and one
> rts_work keeps the before-
> and after-send phases mutually exclusive and the RTS toggles
> ordered, which matters for back-to-back writes and on SMP.
> * The LCR register carries the TX break and RTS bits next to the
> termios bits, so max310x_set_termios() updates only the bits it owns
> instead of writing the register absolutely, and
> max310x_set_rts_ctl_params() settles RTS to the idle level only
> while tx_state is MAX310X_TX_OFF. Both can run while a software
> timed envelope is in flight - serial_core calls ->set_termios()
> without port->lock, and max310x_rs485_config() schedules a
> reconfigure on every TIOCSRS485 - and would otherwise release the
> transceiver mid-character.
> * max310x_shutdown() waits for transmission to finish before powering
> the port down, so close() cannot truncate the final byte.
> On the software path it waits out any in-flight RTS envelope
> (bounded) so the last character and its after-send hold complete.
> On the hardware path it lets the chip FIFO drain and the last
> character clock out of the shift register: tx_empty only reports
> the TX FIFO empty, not the shift register, so without this the
> port could be powered down mid-character. The delay timer and
> rts_work are then cancelled unconditionally: the software/hardware
> decision is recomputed on every reconfigure, so a TIOCSRS485 can
> clear sw_rts_during_tx while an envelope is still in flight, and
> neither may outlive the port. That flag is published in a single
> store for the same reason.
> * max310x_rs485_config() now clamps the delays to the UART core's
> RS485_MAX_RTS_DELAY (100 ms) instead of rejecting them, and
> cancels the pending delay timer when RS485 is disabled.
> * max310x_break_ctl() drives RTS manually for the break duration. The
> chip's auto-RTS only asserts the transceiver while FIFO data is
> shifting out, and a break is not FIFO data, so on the hardware path
> it also disables auto-RTS for the break and restores it when the
> break ends. The software path drives the configured RS485 RTS
> polarity; on the hardware path IRDA.RTSINVERT already inverts the
> RTS_ output stage, so break_state is driven as it is.
That's a lot to do in one commit. Why can't this be broken up into "do
only one thing at a time" patch series?
That might make all of this much more sane and managable and reviewable,
as it is, it is none of that.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v4] serial: max310x: drive RTS in software when hardware delays are too short
2026-09-16 7:10 [PATCH v4] serial: max310x: drive RTS in software when hardware delays are too short Tapio Reijonen
2026-09-23 10:39 ` Greg Kroah-Hartman
@ 2026-09-29 7:13 ` Tapio Reijonen
1 sibling, 0 replies; 3+ messages in thread
From: Tapio Reijonen @ 2026-09-29 7:13 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: linux-kernel, linux-serial, Hugo Villeneuve, Tapio Reijonen
A belated follow-up on the three findings the automated review raised
against v4, since v5 (split into a series, as Greg asked) is about to
be posted and changes course on some of what earlier replies claimed.
The short version: two of the three findings led to changes in v5, and
one of those corrects a claim made in my reply on v3. The third finding
is refuted, and v5 adds a comment at the spot so the reasoning is in
the code rather than in a mail archive.
On "races from dropping port->lock in start_tx/rs485_config":
Right bug, and my earlier assessment was too narrow - both of its
scenarios are real, although the dropped lock is not the mechanism.
->shutdown() runs under port->mutex and ->start_tx() under port->lock,
so they never excluded each other to begin with: the window is the
whole of shutdown(), not the unlock. The same shape exists in the
rs485-disable path, where the sharp end is silent data loss - a
write() racing the disable leaves its bytes queued with no envelope
left to pump them, and a following close() discards them without an
error. In fact this finding and the two shutdown-related findings from
the v3 round collapse into one defect: starting a transmission had no
teardown interlock. v5 adds one (a tx_teardown flag set under
port->lock by shutdown() and the rs485-disable path, checked by
start_tx() on entry and again after the dropped lock is retaken), the
disable path now restarts TX once the reconfigure is applied so the
queued data goes out, and shutdown() also cancels tx_work, which was
previously only cancelled in remove().
On "torn read of the 64-bit one_character_duration":
Valid, and my reply on v3 overreached when it said every value the
driver can hold has a zero upper word. That was board-specific
reasoning stated as a driver-wide claim: with a low reference clock
the no-PLL configuration can win, the baud floor collapses, and a
12-bit frame at single-digit baud does exceed 2^32 ns - at which point
the bounded-shutdown-loop argument from the first round falls with it.
Rather than argue reachability, v5 stores the per-character duration
as unsigned int microseconds: single-copy atomic on 32-bit, so the
torn read is gone by construction, and three of the four users
converted straight back to microseconds anyway.
On "tx_work bypasses the before_send delay":
The premise about the code is correct - nothing in the TXEMPTY path
checks tx_state - but the scenario is unreachable. TXEMPTY latches on
the FIFO's non-empty to empty transition and the IRQ handler's read of
IRQSTS consumes the latch; that read sits upstream of the only path
that ever deasserts RTS, so whenever RTS is low there is no pending
latch left to fire. (If the bit were level-triggered instead, the
handler's read-until-clear loop would never terminate on an idle
port.) A write landing mid-envelope can reset the phase and pay one
extra before-send delay, but RTS is already asserted then, so nothing
is shifted with the transceiver released. v5 adds a comment at the
TXEMPTY handling to that effect.
Of the two pre-existing concerns, shutdown() not cancelling tx_work is
addressed in v5 as part of the interlock work; the remove() ordering
remains out of scope for this series.
Tapio
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-09-29 7:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-16 7:10 [PATCH v4] serial: max310x: drive RTS in software when hardware delays are too short Tapio Reijonen
2026-09-23 10:39 ` Greg Kroah-Hartman
2026-09-29 7:13 ` Tapio Reijonen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®