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.
> > 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.
> > +}
> > +
> > 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/ |
Attachment:
signature.asc
Description: PGP signature
