Messages in this thread | | | Subject | Re: [PATCH 1/3] serial: 8250: Handle UART without interrupt on TEMT using em485 | From | Jiri Slaby <> | Date | Fri, 29 Jan 2021 08:23:31 +0100 |
| |
On 29. 01. 21, 0:36, Eric Tremblay wrote: > The patch introduce the UART_CAP_TEMT capability which is by default > assigned to all 8250 UART since the code assume that device has the > interrupt on TEMT > > In the case where the device does not support it, we calculate the > maximum of time it could take for the transmitter to empty the > shift register. When we get in the situation where we get the > THRE interrupt but the TEMT bit is not set we start the timer > and recall __stop_tx after the delay > > Signed-off-by: Eric Tremblay <etremblay@distech-controls.com> > --- > drivers/tty/serial/8250/8250.h | 1 + > drivers/tty/serial/8250/8250_bcm2835aux.c | 2 +- > drivers/tty/serial/8250/8250_omap.c | 2 +- > drivers/tty/serial/8250/8250_port.c | 89 ++++++++++++++++++++++- > include/linux/serial_8250.h | 2 + > 5 files changed, 93 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h > index 52bb21205bb6..5361b761eed7 100644 > --- a/drivers/tty/serial/8250/8250.h > +++ b/drivers/tty/serial/8250/8250.h > @@ -82,6 +82,7 @@ struct serial8250_config { > #define UART_CAP_MINI (1 << 17) /* Mini UART on BCM283X family lacks: > * STOP PARITY EPAR SPAR WLEN5 WLEN6 > */ > +#define UART_CAP_TEMT (1 << 18) /* UART have interrupt on TEMT */
What about the inversion _NOTEMT? You then set it only on uarts without TEMT and don't need to update every single driver.
> diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c > index fd95860cd661..354faebce885 100644 > --- a/drivers/tty/serial/8250/8250_bcm2835aux.c > +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c > @@ -91,7 +91,7 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev) > return -ENOMEM; > > /* initialize data */ > - up.capabilities = UART_CAP_FIFO | UART_CAP_MINI; > + data->uart.capabilities = UART_CAP_FIFO | UART_CAP_MINI | UART_CAP_TEMT;
This change looks weird and undocumented. Why do you set data->uart suddenly?
Actually, does this build?
> up.port.dev = &pdev->dev; > up.port.regshift = 2; > up.port.type = PORT_16550; > diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c > index 23e0decde33e..1c21ac68ff37 100644 > --- a/drivers/tty/serial/8250/8250_omap.c > +++ b/drivers/tty/serial/8250/8250_omap.c > @@ -1294,7 +1294,7 @@ static int omap8250_probe(struct platform_device *pdev) > up.port.regshift = 2; > up.port.fifosize = 64; > up.tx_loadsz = 64; > - up.capabilities = UART_CAP_FIFO; > + up.capabilities = UART_CAP_FIFO | UART_CAP_TEMT; > #ifdef CONFIG_PM > /* > * Runtime PM is mostly transparent. However to do it right we need to a > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index b0af13074cd3..44a54406e4b4 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -558,8 +558,41 @@ 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; > + > + /* byte size and parity */ > + switch (cflag & CSIZE) { > + case CS5: > + bits = 7; > + break; > + case CS6: > + bits = 8; > + break; > + case CS7: > + bits = 9; > + break; > + default: > + bits = 10; > + break; /* CS8 */ > + } > + > + if (cflag & CSTOPB) > + bits++; > + if (cflag & PARENB) > + bits++; > + > + p->em485->no_temt_delay = bits*1000000/baud; > +} > + > 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) > { > @@ -618,6 +651,18 @@ 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_TEMT)) { > + struct tty_struct *tty = p->port.state->port.tty;
Is this safe? Don't you need a tty reference? Or maybe you need to pass the tty from the TIOCSRS485 ioctl to here.
> + 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; > @@ -649,6 +694,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; > @@ -1494,6 +1540,15 @@ static void start_hrtimer_ms(struct hrtimer *hrt, unsigned long msec) > hrtimer_start(hrt, t, HRTIMER_MODE_REL); > } > > +static void start_hrtimer_us(struct hrtimer *hrt, unsigned long usec) > +{ > + long sec = usec / 1000000; > + long nsec = (usec % 1000000) * 1000; > + ktime_t t = ktime_set(sec, nsec);
Why not ns_to_ktime without all those divisions?
> + > + hrtimer_start(hrt, t, HRTIMER_MODE_REL); > +} > + > static void __stop_tx_rs485(struct uart_8250_port *p) > { > struct uart_8250_em485 *em485 = p->em485; > @@ -1531,8 +1586,18 @@ 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) { > + /* > + * On devices with no interrupt on TEMT available > + * start a timer for a byte time, the timer will recall > + * __stop_tx > + */ > + if (!(p->capabilities & UART_CAP_TEMT) && (lsr & UART_LSR_THRE)) { > + em485->active_timer = &em485->no_temt_timer;
How does this interfere with the current handling of active_timer? You should explain the changed functionality in the commit log. And you don't reset it to NULL in the timer.
> + start_hrtimer_us(&em485->no_temt_timer, em485->no_temt_delay); > + } > return; > + } > > __stop_tx_rs485(p); > } > @@ -1631,6 +1696,25 @@ 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); > + > + spin_unlock_irqrestore(&p->port.lock, flags); > + serial8250_rpm_put(p); > + return HRTIMER_NORESTART; > +} > + > static enum hrtimer_restart serial8250_em485_handle_start_tx(struct hrtimer *t) > { > struct uart_8250_em485 *em485; > @@ -2792,6 +2876,9 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios, > > serial8250_set_divisor(port, baud, quot, frac); > > + if (!(up->capabilities & UART_CAP_TEMT)) > + serial8250_em485_update_temt_delay(up, termios->c_cflag, baud); > + > /* > * LCR DLAB must be set to enable 64-byte FIFO mode. If the FCR > * is written without DLAB set, this mode will be disabled. > diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h > index 9e655055112d..d2c66faff0dd 100644 > --- a/include/linux/serial_8250.h > +++ b/include/linux/serial_8250.h > @@ -79,7 +79,9 @@ struct uart_8250_ops { > struct uart_8250_em485 { > struct hrtimer start_tx_timer; /* "rs485 start tx" timer */ > struct hrtimer stop_tx_timer; /* "rs485 stop tx" timer */ > + struct hrtimer no_temt_timer; /* "rs485 no tempt interrupt" timer */ > struct hrtimer *active_timer; /* pointer to active timer */ > + unsigned int no_temt_delay; /* Value of delay for no TEMT UART */ > struct uart_8250_port *port; /* for hrtimer callbacks */ > unsigned int tx_stopped:1; /* tx is currently stopped */ > }; >
-- js suse labs
| |