Re: [PATCH 1/3] serial: 8250: Handle UART without interrupt on TEMT using em485
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
>> ---
>> 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 = >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*100/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(>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
Re: [PATCH 1/3] serial: 8250: Handle UART without interrupt on TEMT using em485
On 2021-01-29 6:22 a.m., Andy Shevchenko wrote:
> On Thu, Jan 28, 2021 at 06:36:27PM -0500, 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
> You have missed periods in the sentences here and there. Please, check the
> grammar and punctuation everywhere.
>
>> 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
> maximum time
>
>> 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
> __stop_tx()
I will review the grammar and spelling, thanks for mentioning it
>
> ...
>
>> /* initialize data */
>> -up.capabilities = UART_CAP_FIFO | UART_CAP_MINI;
>> +data->uart.capabilities = UART_CAP_FIFO | UART_CAP_MINI | UART_CAP_TEMT;
> I didn't get, if you state that CAP_TEMT is default on all UARTs, why you have
> this?
It's a merge mistake, sorry for that. The next version will use the
reverse capability like Jiri Slaby suggested, there will be no needs to
modify other driver.
>
>> -up.capabilities = UART_CAP_FIFO;
>> +up.capabilities = UART_CAP_FIFO | UART_CAP_TEMT;
> And so this?
>
> ...
>
>> +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++;
> This is repetition of uart_update_timeout(). Find a way to deduplicate.
>
>> +p->em485->no_temt_delay = bits*100/baud;
> Use spaces.
> Is this magic should be defined as HZ_PER_MHZ?
>
>> +}
> ...
>
>> +static void start_hrtimer_us(struct hrtimer *hrt, unsigned long usec)
>> +{
>> +long sec = usec / 100;
>> +long nsec = (usec % 100) * 1000;
>
> USEC_PER_SEC
> NSEC_PER_USEC
>
>> +ktime_t t = ktime_set(sec, nsec);
>> +
>> +hrtimer_start(hrt, t, HRTIMER_MODE_REL);
>> +}
> ...
>
>> +if ((lsr & BOTH_EMPTY) != BOTH_EMPTY) {
>> +/*
>> + * On devices with no interrupt on TEMT available
> "with no TEMT interrupt available"
>
>> + * start a timer for a byte time, the timer will recall
>> + * __stop_tx
> __stop_tx().
>
>> + */
>> +if (!(p->capabilities & UART_CAP_TEMT) && (lsr &
>> UART_LSR_THRE)) {
>> +em485->active_timer = >no_temt_timer;
>> +start_hrtimer_us(>no_temt_timer,
>> em485->no_temt_delay);
>> +}
> Perhaps
> if ((p->capabilities & UART_CAP_TEMT) && (lsr &
> UART_LSR_THRE))
> return;
>
> em485->active_timer = >no_temt_timer;
> start_hrtimer_us(>no_temt_timer,
> em485->no_temt_delay);
>
> ?
I also prefer that form, I will apply it in next version
>
>> return;
>> +}
Re: [PATCH 1/3] serial: 8250: Handle UART without interrupt on TEMT using em485
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
---
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 = >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*100/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(>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(>em485->no_temt_timer, CLOCK_MONOTONIC,
+HRTIMER_MODE_REL);
+ p->em485->no_temt_timer.function =
+
[PATCH 1/3] serial: 8250: Handle UART without interrupt on TEMT using em485
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
---
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 */
#define UART_BUG_QUOT (1 << 0)/* UART has buggy quot LSB */
#define UART_BUG_TXEN (1 << 1)/* UART has buggy TX IIR status */
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;
up.port.dev = >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*100/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(>em485->start_tx_timer, CLOCK_MONOTONIC,
HRTIMER_MODE_REL);
+
+ if (!(p->capabilities & UART_CAP_TEMT)) {
+ struct tty_struct *tty = p->port.state->port.tty;
+
+ serial8250_em485_update_temt_delay(p, tty->termios.c_cflag,
+ tty_get_baud_rate(tty));
+ hrtimer_init(>em485->no_temt_timer, CLOCK_MONOTONIC,
+HRTIMER_MODE_REL);
+ p->em485->no_temt_timer.function =
+ _em485_handle_no_temt;
+ }
+
p->em485->stop_tx_timer.function = _em485_handle_stop_tx;
p->em485->start_tx_timer.function = _em485_handle_start_tx;
p->em485->port = p;
@@ -649,6 +694,7 @@ void serial8250_em485_destroy(struct uart_8250_port *p)
