Re: [PATCH] serial: max310x: drive RTS in software when hardware delays are too short

Hi,

On 7/9/26 13:41, Jiri Slaby wrote:
> On 09. 07. 26, 10:46, 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.
> ...
>> --- a/drivers/tty/serial/max310x.c
>> +++ b/drivers/tty/serial/max310x.c
> ...
>> @@ -324,6 +345,21 @@ static struct uart_driver max310x_uart = {
>>   static DECLARE_BITMAP(max310x_lines, MAX310X_UART_NRMAX);
>> +static ktime_t max310x_get_character_duration(const struct ktermios 
>> *termios,
>> +                          unsigned int baud)
>> +{
>> +    const unsigned int startstop = 2;
>> +    const unsigned int char_bits = startstop +
>> +        (((termios->c_cflag & CSIZE) == CS5) ? 5 : 0) +
>> +        (((termios->c_cflag & CSIZE) == CS6) ? 6 : 0) +
>> +        (((termios->c_cflag & CSIZE) == CS7) ? 7 : 0) +
>> +        (((termios->c_cflag & CSIZE) == CS8) ? 8 : 0) +
>> +        ((termios->c_cflag & PARENB) ? 1 : 0) +
>> +        ((termios->c_cflag & CSTOPB) ? 1 : 0);
> 
> Is this an open-coded tty_get_frame_size()?
> 
It is, yes. Fixed in v2: the helper is gone entirely and
max310x_set_termios() calls tty_get_frame_size() directly. It also fixes
an omission on my side, since tty_get_frame_size() accounts for ADDRB and
my open-coded version did not.
>> +
>> +    return us_to_ktime(DIV_ROUND_UP(USEC_PER_SEC * char_bits, baud));
>> +}
>> +
>>   static u8 max310x_port_read(struct uart_port *port, u8 reg)
>>   {
>>       struct max310x_one *one = to_max310x_port(port);
>> @@ -680,6 +716,39 @@ 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);
>> +}
>> +
>> +/*
>> + * 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;
>> +    unsigned long flags;
>> +    bool rts_on = READ_ONCE(one->tx_state) != MAX310X_TX_OFF;
>> +
>> +    max310x_rts_ctl(port, rts_on ?
>> +            (port->rs485.flags & SER_RS485_RTS_ON_SEND) :
>> +            (port->rs485.flags & SER_RS485_RTS_AFTER_SEND));
>> +
>> +    spin_lock_irqsave(&port->lock, flags);
> 
> We use guard()s these days.
Converted in v2. Besides the two sites you quoted I also converted
max310x_delayed_stop_tx(), which has the same pattern further down.
> 
>> +    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);
>> +    spin_unlock_irqrestore(&port->lock, flags);
>> +}
>> +
>>   static void max310x_handle_rx(struct uart_port *port, unsigned int 
>> rxlen)
>>   {
>>       struct max310x_one *one = to_max310x_port(port);
>> @@ -776,6 +845,75 @@ 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);
>> +    unsigned long flags;
>> +
>> +    spin_lock_irqsave(&one->port.lock, flags);
> 
> guard() on more places.
> 
>> +    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);
>> +        }
>> +    }
>> +    spin_unlock_irqrestore(&one->port.lock, flags);
>> +
>> +    return HRTIMER_NORESTART;
>> +}
> 
> thanks,
I did leave the two bare spin_lock()/spin_unlock() pairs in
max310x_start_tx() and max310x_rs485_config() alone. Both functions are
called with port->lock already held by the caller and only drop it
temporarily around hrtimer_cancel(), which guard() cannot express. Happy
to restructure those if you would rather see them changed.

Thanks for the review.

--
Tapio