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

[Tapio Reijonen]

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