On Wed, Mar 30, 2022 at 02:20:01PM +0300, Ilpo Järvinen wrote: > On Wed, 30 Mar 2022, Uwe Kleine-König wrote: > > > From: Eric Tremblay > > > > 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). ok, if you Cc: me, I'm willing to test on my hardware platform. > > Signed-off-by: Giulio Benetti > > [moved to use added UART_CAP_TEMT] > > Signed-off-by: Heiko Stuebner > > [moved to use added UART_CAP_NOTEMT, improve timeout] > > Signed-off-by: Eric Tremblay > > [rebased to v5.17, making use of tty_get_frame_size] > > Signed-off-by: Uwe Kleine-König > > --- > > 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? Hmm, indeed, even overflows for the usual bits=10. Strange it still worked for me in my tests. Maybe the irq latency is big enough to explain this. > > +} > > + > > 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)? Oh, didn't see this comment. The problem is that the tty might be gone? > > + 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? Yes, that leaked into this patch, IIRC there are a few more code locations that should update lsr_saved_flags. > ...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? There is SER_RS485_RX_DURING_TX ... > 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. *shrug*, it seems maintainers have different mileage regarding fixing typos en passant in patches or making separate patches for them. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |