[PATCH v3 0/3] Handle UART without interrupt on TEMT using em485

[PATCH v3 0/3] Handle UART without interrupt on TEMT using em485

[Uwe Kleine-König]

Hello,

this is a follow up to
https://lore.kernel.org/linux-serial/20210204161158.643-1-etremblay@distech-controls.com

Compared to this v3 the following happend:

 - rebased to v5.17
 - Slightly reworded commit log of patch 2 and 3 (requested by Andy)
 - Use tty_get_frame_size instead of adding another variant.

I tested this series on a Freescale MPC8313 (ARCH=powerpc).

Best regards
Uwe

Eric Tremblay (3):
  serial: 8250: Handle UART without interrupt on TEMT using em485
  serial: 8250: Add UART_CAP_NOTEMT on PORT_16550A_FSL64
  serial: 8250: add compatible for fsl,16550-FIFO64

 drivers/tty/serial/8250/8250.h      |  1 +
 drivers/tty/serial/8250/8250_of.c   |  2 +
 drivers/tty/serial/8250/8250_port.c | 78 +++++++++++++++++++++++++++--
 include/linux/serial_8250.h         |  2 +
 4 files changed, 80 insertions(+), 3 deletions(-)


base-commit: f443e374ae131c168a065ea1748feac6b2e76613
-- 
2.30.2

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

[Uwe Kleine-König]

From: Eric Tremblay <etremblay@distech-controls.com>

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.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
[moved to use added UART_CAP_TEMT]
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
[moved to use added UART_CAP_NOTEMT, improve timeout]
Signed-off-by: Eric Tremblay <etremblay@distech-controls.com>
[rebased to v5.17, making use of tty_get_frame_size]
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 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);
+}
+
 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;
+
+		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;
+
 		/*
-		 * To provide required timeing and allow FIFO transfer,
+		 * To provide required timing and allow FIFO transfer,
 		 * __stop_tx_rs485() must be called only when both FIFO and
 		 * 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) {
+			if (!(p->capabilities & UART_CAP_NOTEMT))
+				/* __stop_tx will be called again once TEMT triggers */
+				return;
+
+			if (!(lsr & UART_LSR_THRE))
+				/* __stop_tx will be called again once THRE triggers */
+				return;
+
+			/*
+			 * On devices with no TEMT interrupt available, start
+			 * a timer for a byte time. The timer will recall
+			 * __stop_tx().
+			 */
+			em485->active_timer = &em485->no_temt_timer;
+			start_hrtimer_ns(&em485->no_temt_timer, em485->no_temt_delay);
 			return;
+		}
 
 		__stop_tx_rs485(p);
 	}
@@ -1633,6 +1681,27 @@ 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) {
+		em485->active_timer = NULL;
+		__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 = container_of(t, struct uart_8250_em485,
@@ -2851,6 +2920,9 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	serial8250_set_divisor(port, baud, quot, frac);
 
+	if (up->capabilities & UART_CAP_NOTEMT)
+		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 ff84a3ed10ea..de135852107c 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 TEMT interrupt" timer */
 	struct hrtimer		*active_timer;  /* pointer to active timer */
+	unsigned long		no_temt_delay;  /* Delay for no_temt_timer */
 	struct uart_8250_port	*port;          /* for hrtimer callbacks */
 	unsigned int		tx_stopped:1;	/* tx is currently stopped */
 };
-- 
2.30.2

[PATCH v3 2/3] serial: 8250: Add UART_CAP_NOTEMT on PORT_16550A_FSL64

[Uwe Kleine-König]

From: Eric Tremblay <etremblay@distech-controls.com>

The Freescale variant of the 16550A doesn't have an interrupt on TEMT
available when using the FIFO mode.

Signed-off-by: Eric Tremblay <etremblay@distech-controls.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/serial/8250/8250_port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 0af13b4c76a0..b9f252d24207 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -263,7 +263,7 @@ static const struct serial8250_config uart_config[] = {
 		.tx_loadsz	= 63,
 		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 |
 				  UART_FCR7_64BYTE,
-		.flags		= UART_CAP_FIFO,
+		.flags		= UART_CAP_FIFO | UART_CAP_NOTEMT,
 	},
 	[PORT_RT2880] = {
 		.name		= "Palmchip BK-3103",
-- 
2.30.2

[PATCH v3 3/3] serial: 8250: add compatible for fsl,16550-FIFO64

[Uwe Kleine-König]

From: Eric Tremblay <etremblay@distech-controls.com>

Signed-off-by: Eric Tremblay <etremblay@distech-controls.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/serial/8250/8250_of.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
index be8626234627..5a699a1aa79c 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -326,6 +326,8 @@ static const struct of_device_id of_platform_serial_table[] = {
 		.data = (void *)PORT_ALTR_16550_F64, },
 	{ .compatible = "altr,16550-FIFO128",
 		.data = (void *)PORT_ALTR_16550_F128, },
+	{ .compatible = "fsl,16550-FIFO64",
+		.data = (void *)PORT_16550A_FSL64, },
 	{ .compatible = "mediatek,mtk-btif",
 		.data = (void *)PORT_MTK_BTIF, },
 	{ .compatible = "mrvl,mmp-uart",
-- 
2.30.2

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5714 bytes --]

On Wed, 30 Mar 2022, Uwe Kleine-König wrote:

> From: Eric Tremblay <etremblay@distech-controls.com>
> 
> 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).

> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> [moved to use added UART_CAP_TEMT]
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> [moved to use added UART_CAP_NOTEMT, improve timeout]
> Signed-off-by: Eric Tremblay <etremblay@distech-controls.com>
> [rebased to v5.17, making use of tty_get_frame_size]
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  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?

> +}
> +
>  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)?

> +		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?
...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? 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.

-- 
 i.

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 6951 bytes --]

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@distech-controls.com>
> > 
> > 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@micronovasrl.com>
> > [moved to use added UART_CAP_TEMT]
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > [moved to use added UART_CAP_NOTEMT, improve timeout]
> > Signed-off-by: Eric Tremblay <etremblay@distech-controls.com>
> > [rebased to v5.17, making use of tty_get_frame_size]
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 7320 bytes --]

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@distech-controls.com>
> > > 
> > > 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@micronovasrl.com>
> > > [moved to use added UART_CAP_TEMT]
> > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > [moved to use added UART_CAP_NOTEMT, improve timeout]
> > > Signed-off-by: Eric Tremblay <etremblay@distech-controls.com>
> > > [rebased to v5.17, making use of tty_get_frame_size]
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  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.

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4630 bytes --]

Hi Greg,

This change now appears in your tty-next tree. As you seem to have missed 
there is an obvious problem with it, I'm asking which direction I should 
take to fix it.

My RS485 patchset requires frame timing information also for other 
purpose besides notemt and it seems some other drivers (and even serial 
core itself) seem to care about frame time so I was aiming to make it 
available more generally rather than per-purpose like it is now.

Should I just submit a patch which makes the div 64-bit fixing this change 
or should I pursue the alternative approach that is part of my RS485 
patchset (patches 1-2) that generalizes availability of the frame timing 
information and effectively reverts the addition of the extra notemt timer 
added by this change?


On Thu, 31 Mar 2022, Ilpo Järvinen wrote:
> 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@distech-controls.com>
> > > > 
> > > > 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).
> > >
> > > > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > > > [moved to use added UART_CAP_TEMT]
> > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > > [moved to use added UART_CAP_NOTEMT, improve timeout]
> > > > Signed-off-by: Eric Tremblay <etremblay@distech-controls.com>
> > > > [rebased to v5.17, making use of tty_get_frame_size]
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > >  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.


-- 
 i.

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

[Greg Kroah-Hartman]

On Tue, Apr 19, 2022 at 11:09:56AM +0300, Ilpo Järvinen wrote:
> Hi Greg,
> 
> This change now appears in your tty-next tree.

What change?  Please never top-post.

> As you seem to have missed 
> there is an obvious problem with it, I'm asking which direction I should 
> take to fix it.

Send a fix!  You don't need my permission to do so.

greg k-h

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]

On Tue, 19 Apr 2022, Greg Kroah-Hartman wrote:

> On Tue, Apr 19, 2022 at 11:09:56AM +0300, Ilpo Järvinen wrote:
> > Hi Greg,
> > 
> > This change now appears in your tty-next tree.
> 
> What change?  Please never top-post.

f6f586102add59d57bcc6eea06fdeaae11bb17a1 (serial: 8250: Handle UART 
without interrupt on TEMT using em485).

> > As you seem to have missed 
> > there is an obvious problem with it, I'm asking which direction I should 
> > take to fix it.
> 
> Send a fix!  You don't need my permission to do so.

Yes, I know I don't need permission :-).

What I asked is whether I should provide:
  a) a minimal fix to the issue in this particular change
or
  b) send patches that replace this notemt approach with the another
     that I believe is better than this one (*)
?

(*) In case you want to see the another approach before answering, it's 
part of my RS485 patchset (patches 1-2) [1]. My solution doesn't need
the extra notemt timer and also allows drivers easy access to frame
timing information (rather than them storing it per purpose).


-- 
 i.

[1] https://lore.kernel.org/linux-serial/20220411083321.9131-3-ilpo.jarvinen@linux.intel.com/T/#u

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

[Greg Kroah-Hartman]

On Tue, Apr 19, 2022 at 01:36:28PM +0300, Ilpo Järvinen wrote:
> On Tue, 19 Apr 2022, Greg Kroah-Hartman wrote:
> 
> > On Tue, Apr 19, 2022 at 11:09:56AM +0300, Ilpo Järvinen wrote:
> > > Hi Greg,
> > > 
> > > This change now appears in your tty-next tree.
> > 
> > What change?  Please never top-post.
> 
> f6f586102add59d57bcc6eea06fdeaae11bb17a1 (serial: 8250: Handle UART 
> without interrupt on TEMT using em485).
> 
> > > As you seem to have missed 
> > > there is an obvious problem with it, I'm asking which direction I should 
> > > take to fix it.
> > 
> > Send a fix!  You don't need my permission to do so.
> 
> Yes, I know I don't need permission :-).
> 
> What I asked is whether I should provide:
>   a) a minimal fix to the issue in this particular change
> or
>   b) send patches that replace this notemt approach with the another
>      that I believe is better than this one (*)
> ?

Or c) a patch to revert this now and then submit a correct series later?

I prefer c)

thanks,

greg k-h