Re: [PATCH 1/3] serial: 8250: Handle UART without interrupt on TEMT using em485

From: Eric Tremblay
Date: Mon Feb 01 2021 - 19:17:23 EST

Next message: Colin Xu: "Re: [PATCH RESEND v3] virtio-input: add multi-touch support"
Previous message: Alex Elder: "Re: [PATCH net 3/4] net: ipa: use the right accessor in ipa_endpoint_status_skip()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2021-01-29 2:23 a.m., Jiri Slaby wrote:
> 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@xxxxxxxxxxxxxxxxxxxx>
>> ---
>> 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.
That's a good Idea, I will use that in the next version.
>
>> 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?

This is a silly merge mistake, sorry about that. However, it will not be there

in the next version with the _NOTEMT capability.

>
>>       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.
I saw that there was some place in the code where the tty_struct or the termios struct
was accessed that way, I may have done a wrong assumption. To get the TTY struct from the IOCTL,
we will need to modify the rs485_config() callback, I'm not sure I want to go there.

Is using tty_port_initialized() before and returning an error on failure would be a sufficient
safety check ?
>
>> +        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?

I will change the timer to _ns and use that function, thanks.

>
>> +
>> +    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.
I will add that information in the commit message thanks.
>
>> +                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 */
>> };
>>
>
>

Next message: Colin Xu: "Re: [PATCH RESEND v3] virtio-input: add multi-touch support"
Previous message: Alex Elder: "Re: [PATCH net 3/4] net: ipa: use the right accessor in ipa_endpoint_status_skip()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]