Re: [PATCH v2 1/3] serial: 8250: Handle UART without interrupt on TEMT using em485

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 4 Feb 2021, Eric Tremblay wrote:

> The patch introduce the UART_CAP_NOTEMT capability. The capability
> indicate that the UART doesn't have an interrupt available on TEMT.
> 
> In the case where the device does not support it, we calculate the
> maximum time it could take for the transmitter to empty the
> shift register. When we get in the situation where we get the
> THRE interrupt, we check if the TEMT bit is set. If it's not, we start
> the a timer and recall __stop_tx() after the delay.
> 
> The transmit sequence is a bit modified when the capability is set. The
> new timer is used between the last interrupt(THRE) and a potential
> stop_tx timer.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> [moved to use added UART_CAP_TEMT]
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> [moved to use added UART_CAP_NOTEMT, improve timeout]
> Signed-off-by: Eric Tremblay <etremblay@distech-controls.com>
> ---

> @@ -1531,8 +1560,19 @@ static inline void __stop_tx(struct uart_8250_port *p)
>  		 * shift register are empty. It is for device driver to enable
>  		 * interrupt on TEMT.
>  		 */
> -		if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
> +		if ((lsr & BOTH_EMPTY) != BOTH_EMPTY) {
> +			if (!(p->capabilities & UART_CAP_NOTEMT))
> +				return;
> +
> +			/*
> +			 * On devices with no TEMT interrupt available, start
> +			 * a timer for a byte time. The timer will recall
> +			 * __stop_tx().
> +			 */
> +			em485->active_timer = &em485->no_temt_timer;
> +			start_hrtimer_ns(&em485->no_temt_timer, em485->no_temt_delay);

Is it ok to start the timer here without first confirming UART_LSR_THRE?

>  			return;
> +		}
>  
>  		__stop_tx_rs485(p);
>  	}
> @@ -1631,6 +1671,27 @@ static inline void start_tx_rs485(struct uart_port *port)
>  	__start_tx(port);
>  }
>  
> +static enum hrtimer_restart serial8250_em485_handle_no_temt(struct hrtimer *t)
> +{
> +	struct uart_8250_em485 *em485;
> +	struct uart_8250_port *p;
> +	unsigned long flags;
> +
> +	em485 = container_of(t, struct uart_8250_em485, no_temt_timer);
> +	p = em485->port;
> +
> +	serial8250_rpm_get(p);
> +	spin_lock_irqsave(&p->port.lock, flags);
> +	if (em485->active_timer == &em485->no_temt_timer) {
> +		__stop_tx(p);
> +		em485->active_timer = NULL;

If BOTH_EMPTY is still not set when calling __stop_tx from here,
__stop_tx ends up just starting the timer again and the timer won't do 
anything useful when expiring because active_timer is now NULL.

> +	}
> +
> +	spin_unlock_irqrestore(&p->port.lock, flags);
> +	serial8250_rpm_put(p);
> +	return HRTIMER_NORESTART;
> +}


-- 
 i.