On Wed, 30 Mar 2022, Uwe Kleine-König wrote:
> 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 <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).
>
> ok, if you Cc: me, I'm willing to test on my hardware platform.
I'll, although I put you as Suggested-by so I think git send-email would
pick that up anyway.
> > > 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?
>
> 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.
Latency likely covers for most of it to begin with and it typically still
has a non-zero delay. If TEMT is not yet set when the notemt timer
expires, your code just keeps rearming the timer until the TEMT gets set.
And on 64-bit archs, NSEC_PER_SEC is long.
> > > +}
> > > +
> > > 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?
I don't claim any expertise on this, mostly just pointing to that old
comment. After all, we're here inside tty's ioctl so I'm not sure if
such a problem can occur (but there are init paths besides the ioctl
one, I've not tried to check them).
In my approach it won't matter though because I made the frame_time
available so I've no need to get it from termios.
> > > + 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 ...
Ah, indeed. Somehow I always end up thinking this em485 RTS toggling
implies half-duplex.
--
i.
