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

[Jiri Slaby <jirislaby@kernel.org>]

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()?

> +
> +	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.

> +	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,
-- 
js
suse labs