On Wed, 30 Mar 2022, Uwe Kleine-König wrote:
> From: Eric Tremblay <etremblay@xxxxxxxxxxxxxxxxxxxx>
>
> Introduce the UART_CAP_NOTEMT capability. The capability indicates 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.
As a general note on this patch, I've also made a version of this patch
which I intended to send among my dw rs485 v2 patchset once the merge
window is over. I believe my approach is cleaner than this one. It is
based on your suggestion on simply taking advantage of stop_tx_timer.
In addition, I added frame_time into uart_port which removes the need
for drivers to calculate the timing per usecase themselves (I believe
frame_time could replace the timeout in uart_port entirely).
> Signed-off-by: Giulio Benetti <giulio.benetti@xxxxxxxxxxxxxxxx>
> [moved to use added UART_CAP_TEMT]
> Signed-off-by: Heiko Stuebner <heiko.stuebner@xxxxxxxxxxxxxxxxxxxxx>
> [moved to use added UART_CAP_NOTEMT, improve timeout]
> Signed-off-by: Eric Tremblay <etremblay@xxxxxxxxxxxxxxxxxxxx>
> [rebased to v5.17, making use of tty_get_frame_size]
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> ---
> drivers/tty/serial/8250/8250.h | 1 +
> drivers/tty/serial/8250/8250_port.c | 76 ++++++++++++++++++++++++++++-
> include/linux/serial_8250.h | 2 +
> 3 files changed, 77 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index db784ace25d8..39ffeb37786f 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -83,6 +83,7 @@ struct serial8250_config {
> #define UART_CAP_MINI BIT(17) /* Mini UART on BCM283X family lacks:
> * STOP PARITY EPAR SPAR WLEN5 WLEN6
> */
> +#define UART_CAP_NOTEMT BIT(18) /* UART without interrupt on TEMT available */
>
> #define UART_BUG_QUOT BIT(0) /* UART has buggy quot LSB */
> #define UART_BUG_TXEN BIT(1) /* UART has buggy TX IIR status */
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 3b12bfc1ed67..0af13b4c76a0 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -563,8 +563,21 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
> }
> }
>
> +static inline void serial8250_em485_update_temt_delay(struct uart_8250_port *p,
> + unsigned int cflag, unsigned int baud)
> +{
> + unsigned int bits;
> +
> + if (!p->em485)
> + return;
> +
> + bits = tty_get_frame_size(cflag);
> + p->em485->no_temt_delay = DIV_ROUND_UP(bits * NSEC_PER_SEC, baud);
This is guaranteed to overflow on some archs?
> +}
> +
> static enum hrtimer_restart serial8250_em485_handle_start_tx(struct hrtimer *t);
> static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t);
> +static enum hrtimer_restart serial8250_em485_handle_no_temt(struct hrtimer *t);
>
> void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
> {
> @@ -623,6 +636,16 @@ static int serial8250_em485_init(struct uart_8250_port *p)
> HRTIMER_MODE_REL);
> hrtimer_init(&p->em485->start_tx_timer, CLOCK_MONOTONIC,
> HRTIMER_MODE_REL);
> +
> + if (p->capabilities & UART_CAP_NOTEMT) {
> + struct tty_struct *tty = p->port.state->port.tty;
Is this safe (it was commented already by Jiri against one of Eric's
patchsets)?
> + serial8250_em485_update_temt_delay(p, tty->termios.c_cflag,
> + tty_get_baud_rate(tty));
> + hrtimer_init(&p->em485->no_temt_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + p->em485->no_temt_timer.function = &serial8250_em485_handle_no_temt;
> + }
> +
> p->em485->stop_tx_timer.function = &serial8250_em485_handle_stop_tx;
> p->em485->start_tx_timer.function = &serial8250_em485_handle_start_tx;
> p->em485->port = p;
> @@ -654,6 +677,7 @@ void serial8250_em485_destroy(struct uart_8250_port *p)
>
> hrtimer_cancel(&p->em485->start_tx_timer);
> hrtimer_cancel(&p->em485->stop_tx_timer);
> + hrtimer_cancel(&p->em485->no_temt_timer);
>
> kfree(p->em485);
> p->em485 = NULL;
> @@ -1496,6 +1520,11 @@ static void start_hrtimer_ms(struct hrtimer *hrt, unsigned long msec)
> hrtimer_start(hrt, ms_to_ktime(msec), HRTIMER_MODE_REL);
> }
>
> +static void start_hrtimer_ns(struct hrtimer *hrt, unsigned long nsec)
> +{
> + hrtimer_start(hrt, ns_to_ktime(nsec), HRTIMER_MODE_REL);
> +}
> +
> static void __stop_tx_rs485(struct uart_8250_port *p)
> {
> struct uart_8250_em485 *em485 = p->em485;
> @@ -1527,14 +1556,33 @@ static inline void __stop_tx(struct uart_8250_port *p)
>
> if (em485) {
> unsigned char lsr = serial_in(p, UART_LSR);
> +
> + p->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
This change doesn't belong to this patch. It's an independent fix?
...I'm not entirely sure it's fixing something though. After all, we're
talking about half-duplex here so it should not have those rx related
flags that need to be saved? It doesn't hurt though even if possibly not
strictly mandatory so I'm not strictly against it.
> +
> /*
> - * To provide required timeing and allow FIFO transfer,
> + * To provide required timing and allow FIFO transfer,
This too is independent change that should be in its own patch.
--
i.
