LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw.
@ 2018-06-01 12:40 Giulio Benetti
  2018-06-01 12:40 ` [PATCH 1/8] serial: 8250_dw: add em485 support Giulio Benetti
                   ` (8 more replies)
  0 siblings, 9 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-01 12:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, Andy Shevchenko, Kees Cook, Matthias Brugger,
	Allen Pais, Sean Young, Ed Blake, Stefan Potyra, Philipp Zabel,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Rafael Gago, Joel Stanley, Sean Wang, linux-serial, linux-kernel,
	Giulio Benetti

Need to handle rs485 with 8250_dw port.

Use existing em485 emulation layer for 8250 taking care to fix some bug
and taking care especially of RTS_AFTER_SEND case.

Giulio Benetti (8):
  serial: 8250_dw: add em485 support
  serial: 8250_dw: allow enable rs485 at boot time
  serial: 8250: Copy em485 from port to real port.
  serial: 8250: Handle case port doesn't have TEMT interrupt using
    em485.
  serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and
    RTS_AFTER_SEND
  serial: 8250: Copy mctrl when register port.
  serial: 8250: Make em485_rts_after_send() set mctrl according to rts
    state.
  serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and
    RTS_AFTER_SEND set.

 drivers/tty/serial/8250/8250.h      |  2 +-
 drivers/tty/serial/8250/8250_core.c |  2 ++
 drivers/tty/serial/8250/8250_dw.c   | 41 ++++++++++++++++++++++++++++-
 drivers/tty/serial/8250/8250_omap.c |  2 +-
 drivers/tty/serial/8250/8250_port.c | 33 ++++++++++++++++-------
 drivers/tty/serial/serial_core.c    | 12 ++++++++-
 include/linux/serial_8250.h         |  1 +
 7 files changed, 79 insertions(+), 14 deletions(-)

-- 
2.17.0

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 1/8] serial: 8250_dw: add em485 support
  2018-06-01 12:40 [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw Giulio Benetti
@ 2018-06-01 12:40 ` Giulio Benetti
  2018-06-01 12:40 ` [PATCH 2/8] serial: 8250_dw: allow enable rs485 at boot time Giulio Benetti
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-01 12:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, Andy Shevchenko, Kees Cook, Matthias Brugger,
	Allen Pais, Sean Young, Ed Blake, Stefan Potyra, Philipp Zabel,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Rafael Gago, Joel Stanley, Sean Wang, linux-serial, linux-kernel,
	Giulio Benetti

Need to use rs485 transceiver so let's use existing em485 485 emulation
layer on top of 8250.

Add rs485_config callback to port.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/8250/8250_dw.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 6fcdb90f616a..45366e6e5411 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -314,6 +314,36 @@ static void dw8250_set_ldisc(struct uart_port *p, struct ktermios *termios)
 	serial8250_do_set_ldisc(p, termios);
 }
 
+static int dw8250_rs485_config(struct uart_port *p,
+			       struct serial_rs485 *rs485)
+{
+	struct uart_8250_port *up = up_to_u8250p(p);
+
+	/* Clamp the delays to [0, 100ms] */
+	rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
+	rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send, 100U);
+
+	p->rs485 = *rs485;
+
+	/*
+	 * Both serial8250_em485_init and serial8250_em485_destroy
+	 * are idempotent
+	 */
+	if (rs485->flags & SER_RS485_ENABLED) {
+		int ret = serial8250_em485_init(up);
+
+		if (ret) {
+			rs485->flags &= ~SER_RS485_ENABLED;
+			p->rs485.flags &= ~SER_RS485_ENABLED;
+		}
+		return ret;
+	}
+
+	serial8250_em485_destroy(up);
+
+	return 0;
+}
+
 /*
  * dw8250_fallback_dma_filter will prevent the UART from getting just any free
  * channel on platforms that have DMA engines, but don't have any channels
@@ -449,6 +479,7 @@ static int dw8250_probe(struct platform_device *pdev)
 	p->serial_out	= dw8250_serial_out;
 	p->set_ldisc	= dw8250_set_ldisc;
 	p->set_termios	= dw8250_set_termios;
+	p->rs485_config = dw8250_rs485_config;
 
 	p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
 	if (!p->membase)
-- 
2.17.0

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 2/8] serial: 8250_dw: allow enable rs485 at boot time
  2018-06-01 12:40 [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw Giulio Benetti
  2018-06-01 12:40 ` [PATCH 1/8] serial: 8250_dw: add em485 support Giulio Benetti
@ 2018-06-01 12:40 ` Giulio Benetti
  2018-06-01 12:40 ` [PATCH 3/8] serial: 8250: Copy em485 from port to real port Giulio Benetti
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-01 12:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, Andy Shevchenko, Kees Cook, Matthias Brugger,
	Allen Pais, Sean Young, Ed Blake, Stefan Potyra, Philipp Zabel,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Rafael Gago, Joel Stanley, Sean Wang, linux-serial, linux-kernel,
	Giulio Benetti

If "linux,rs485-enabled-at-boot-time" is specified need to setup 485
in probe function.

Call uart_get_rs485_mode() to get rs485 configuration, then call
rs485_config() callback directly to setup port as rs485.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/8250/8250_dw.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 45366e6e5411..0f8b4da03d4e 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -582,8 +582,11 @@ static int dw8250_probe(struct platform_device *pdev)
 	if (data->uart_16550_compatible)
 		p->handle_irq = NULL;
 
-	if (!data->skip_autocfg)
+	if (!data->skip_autocfg) {
 		dw8250_setup_port(p);
+		uart_get_rs485_mode(dev, &p->rs485);
+		dw8250_rs485_config(p, &p->rs485);
+	}
 
 	/* If we have a valid fifosize, try hooking up DMA */
 	if (p->fifosize) {
-- 
2.17.0

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 3/8] serial: 8250: Copy em485 from port to real port.
  2018-06-01 12:40 [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw Giulio Benetti
  2018-06-01 12:40 ` [PATCH 1/8] serial: 8250_dw: add em485 support Giulio Benetti
  2018-06-01 12:40 ` [PATCH 2/8] serial: 8250_dw: allow enable rs485 at boot time Giulio Benetti
@ 2018-06-01 12:40 ` Giulio Benetti
  2018-06-04 10:13   ` Andy Shevchenko
  2018-06-01 12:40 ` [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485 Giulio Benetti
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 50+ messages in thread
From: Giulio Benetti @ 2018-06-01 12:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, Andy Shevchenko, Kees Cook, Matthias Brugger,
	Allen Pais, Sean Young, Ed Blake, Stefan Potyra, Philipp Zabel,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Rafael Gago, Joel Stanley, Sean Wang, linux-serial, linux-kernel,
	Giulio Benetti

em485 gets lost during serial8250_register_8250_port().

Copy em485 to final uart port.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/8250/8250_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 9342fc2ee7df..c8c2b260c681 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -994,6 +994,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 		uart->port.rs485_config	= up->port.rs485_config;
 		uart->port.rs485	= up->port.rs485;
 		uart->dma		= up->dma;
+		uart->em485		= up->em485;
 
 		/* Take tx_loadsz from fifosize if it wasn't set separately */
 		if (uart->port.fifosize && !uart->tx_loadsz)
-- 
2.17.0

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.
  2018-06-01 12:40 [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw Giulio Benetti
                   ` (2 preceding siblings ...)
  2018-06-01 12:40 ` [PATCH 3/8] serial: 8250: Copy em485 from port to real port Giulio Benetti
@ 2018-06-01 12:40 ` Giulio Benetti
  2018-06-04 10:17   ` Andy Shevchenko
  2018-06-04 17:40   ` Matwey V. Kornilov
  2018-06-01 12:40 ` [PATCH 5/8] serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and RTS_AFTER_SEND Giulio Benetti
                   ` (4 subsequent siblings)
  8 siblings, 2 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-01 12:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, Andy Shevchenko, Kees Cook, Matthias Brugger,
	Allen Pais, Sean Young, Ed Blake, Stefan Potyra, Philipp Zabel,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Rafael Gago, Joel Stanley, Sean Wang, linux-serial, linux-kernel,
	Giulio Benetti

Some 8250 ports only have TEMT interrupt, so current implementation
can't work for ports without it. The only chance to make it work is to
loop-read on LSR register.

With NO TEMT interrupt check if both TEMT and THRE are set looping on
LSR register.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/8250/8250.h      |  2 +-
 drivers/tty/serial/8250/8250_dw.c   |  2 +-
 drivers/tty/serial/8250/8250_omap.c |  2 +-
 drivers/tty/serial/8250/8250_port.c | 26 ++++++++++++++++++--------
 include/linux/serial_8250.h         |  1 +
 5 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ebfb0bd5bef5..5c6ae5f69432 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -136,7 +136,7 @@ void serial8250_rpm_put(struct uart_8250_port *p);
 void serial8250_rpm_get_tx(struct uart_8250_port *p);
 void serial8250_rpm_put_tx(struct uart_8250_port *p);
 
-int serial8250_em485_init(struct uart_8250_port *p);
+int serial8250_em485_init(struct uart_8250_port *p, bool has_temt_isr);
 void serial8250_em485_destroy(struct uart_8250_port *p);
 
 static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 0f8b4da03d4e..888280ff5451 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -330,7 +330,7 @@ static int dw8250_rs485_config(struct uart_port *p,
 	 * are idempotent
 	 */
 	if (rs485->flags & SER_RS485_ENABLED) {
-		int ret = serial8250_em485_init(up);
+		int ret = serial8250_em485_init(up, false);
 
 		if (ret) {
 			rs485->flags &= ~SER_RS485_ENABLED;
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 624b501fd253..ab483c8b30c8 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -725,7 +725,7 @@ static int omap_8250_rs485_config(struct uart_port *port,
 	 * are idempotent
 	 */
 	if (rs485->flags & SER_RS485_ENABLED) {
-		int ret = serial8250_em485_init(up);
+		int ret = serial8250_em485_init(up, true);
 
 		if (ret) {
 			rs485->flags &= ~SER_RS485_ENABLED;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 95833cbc4338..e14badbbf181 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -599,15 +599,16 @@ EXPORT_SYMBOL_GPL(serial8250_rpm_put);
 /**
  *	serial8250_em485_init() - put uart_8250_port into rs485 emulating
  *	@p:	uart_8250_port port instance
+ *	@p:	bool specify if 8250 port has TEMT interrupt or not
  *
  *	The function is used to start rs485 software emulating on the
  *	&struct uart_8250_port* @p. Namely, RTS is switched before/after
  *	transmission. The function is idempotent, so it is safe to call it
  *	multiple times.
  *
- *	The caller MUST enable interrupt on empty shift register before
- *	calling serial8250_em485_init(). This interrupt is not a part of
- *	8250 standard, but implementation defined.
+ *	If has_temt_isr is passed as true, the caller MUST enable interrupt
+ *	on empty shift register before calling serial8250_em485_init().
+ *	This interrupt is not a part of	8250 standard, but implementation defined.
  *
  *	The function is supposed to be called from .rs485_config callback
  *	or from any other callback protected with p->port.lock spinlock.
@@ -616,7 +617,7 @@ EXPORT_SYMBOL_GPL(serial8250_rpm_put);
  *
  *	Return 0 - success, -errno - otherwise
  */
-int serial8250_em485_init(struct uart_8250_port *p)
+int serial8250_em485_init(struct uart_8250_port *p, bool has_temt_isr)
 {
 	if (p->em485)
 		return 0;
@@ -633,6 +634,7 @@ int serial8250_em485_init(struct uart_8250_port *p)
 	p->em485->start_tx_timer.function = &serial8250_em485_handle_start_tx;
 	p->em485->port = p;
 	p->em485->active_timer = NULL;
+	p->em485->has_temt_isr = has_temt_isr;
 	serial8250_em485_rts_after_send(p);
 
 	return 0;
@@ -1517,11 +1519,19 @@ static inline void __stop_tx(struct uart_8250_port *p)
 		/*
 		 * To provide required timeing 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.
+		 * shift register are empty. If 8250 port supports it,
+		 * it is for device driver to enable interrupt on TEMT.
+		 * Otherwise must loop-read until TEMT and THRE flags are set.
 		 */
-		if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
-			return;
+		if (em485->has_temt_isr) {
+			if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
+				return;
+		} else {
+			while ((lsr & BOTH_EMPTY) != BOTH_EMPTY) {
+				lsr = serial_in(p, UART_LSR);
+				cpu_relax();
+			}
+		}
 
 		em485->active_timer = NULL;
 
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index a27ef5f56431..9b13cf59726b 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -83,6 +83,7 @@ struct uart_8250_em485 {
 	struct hrtimer		start_tx_timer; /* "rs485 start tx" timer */
 	struct hrtimer		stop_tx_timer;  /* "rs485 stop tx" timer */
 	struct hrtimer		*active_timer;  /* pointer to active timer */
+	bool			has_temt_isr;	/* say if 8250 has TEMT interrupt or no */
 	struct uart_8250_port	*port;          /* for hrtimer callbacks */
 };
 
-- 
2.17.0

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 5/8] serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and RTS_AFTER_SEND
  2018-06-01 12:40 [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw Giulio Benetti
                   ` (3 preceding siblings ...)
  2018-06-01 12:40 ` [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485 Giulio Benetti
@ 2018-06-01 12:40 ` Giulio Benetti
  2018-06-01 12:40 ` [PATCH 6/8] serial: 8250: Copy mctrl when register port Giulio Benetti
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-01 12:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, Andy Shevchenko, Kees Cook, Matthias Brugger,
	Allen Pais, Sean Young, Ed Blake, Stefan Potyra, Philipp Zabel,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Rafael Gago, Joel Stanley, Sean Wang, linux-serial, linux-kernel,
	Giulio Benetti

If rs485 is on and RTS_AFTER_SEND it means that in idle state rts pin
must be asserted, othwerwise rs485 transceiver will enter tx state.
dw8250 when clocks are stopped keeps rts line de-asserted(high),
resulting in keeping rs485 transceiver in tx state when port is idle.

Check if rs485 is on with RTS_AFTER_SEND set, if so return -EBUSY in
rpm_suspend,

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/8250/8250_dw.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 888280ff5451..6b0ee6dc8ad0 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -668,6 +668,11 @@ static int dw8250_resume(struct device *dev)
 static int dw8250_runtime_suspend(struct device *dev)
 {
 	struct dw8250_data *data = dev_get_drvdata(dev);
+	struct uart_8250_port *up = serial8250_get_port(data->line);
+	struct uart_port *p = &up->port;
+
+	if (p->rs485.flags & (SER_RS485_ENABLED | SER_RS485_RTS_AFTER_SEND))
+		return -EBUSY;
 
 	if (!IS_ERR(data->clk))
 		clk_disable_unprepare(data->clk);
-- 
2.17.0

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 6/8] serial: 8250: Copy mctrl when register port.
  2018-06-01 12:40 [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw Giulio Benetti
                   ` (4 preceding siblings ...)
  2018-06-01 12:40 ` [PATCH 5/8] serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and RTS_AFTER_SEND Giulio Benetti
@ 2018-06-01 12:40 ` Giulio Benetti
  2018-06-06 14:31   ` Aaron Sierra
  2018-06-01 12:40 ` [PATCH 7/8] serial: 8250: Make em485_rts_after_send() set mctrl according to rts state Giulio Benetti
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 50+ messages in thread
From: Giulio Benetti @ 2018-06-01 12:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, Andy Shevchenko, Kees Cook, Matthias Brugger,
	Allen Pais, Sean Young, Ed Blake, Stefan Potyra, Philipp Zabel,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Rafael Gago, Joel Stanley, Sean Wang, linux-serial, linux-kernel,
	Giulio Benetti

RS485 can modify mctrl on startup, especially when RTS_AFTER_SEND is on
TIOCM_RTS is set, then need to keep it set when registering port.

Copy mctrl to new port too.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/8250/8250_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index c8c2b260c681..c8e62fbd6570 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -993,6 +993,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 		uart->port.unthrottle	= up->port.unthrottle;
 		uart->port.rs485_config	= up->port.rs485_config;
 		uart->port.rs485	= up->port.rs485;
+		uart->port.mctrl	= up->port.mctrl;
 		uart->dma		= up->dma;
 		uart->em485		= up->em485;
 
-- 
2.17.0

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 7/8] serial: 8250: Make em485_rts_after_send() set mctrl according to rts state.
  2018-06-01 12:40 [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw Giulio Benetti
                   ` (5 preceding siblings ...)
  2018-06-01 12:40 ` [PATCH 6/8] serial: 8250: Copy mctrl when register port Giulio Benetti
@ 2018-06-01 12:40 ` Giulio Benetti
  2018-06-01 12:40 ` [PATCH 8/8] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set Giulio Benetti
  2018-06-04 10:12 ` [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw Andy Shevchenko
  8 siblings, 0 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-01 12:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, Andy Shevchenko, Kees Cook, Matthias Brugger,
	Allen Pais, Sean Young, Ed Blake, Stefan Potyra, Philipp Zabel,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Rafael Gago, Joel Stanley, Sean Wang, linux-serial, linux-kernel,
	Giulio Benetti

When rs485 enabled and RTS_AFTER_SEND set on startup, need to preserve
mctrl status, because later functions will call set_mctrl passing
port->mctrl=0 overriding rts status, resulting in rts pin in
transmission when idle.

Make mctrl reflect rts pin state.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/8250/8250_port.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index e14badbbf181..8432445c80e5 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -562,10 +562,13 @@ static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
 {
 	unsigned char mcr = serial8250_in_MCR(p);
 
-	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
+	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND) {
 		mcr |= UART_MCR_RTS;
-	else
+		p->port.mctrl |= TIOCM_RTS;
+	} else {
 		mcr &= ~UART_MCR_RTS;
+		p->port.mctrl &= ~TIOCM_RTS;
+	}
 	serial8250_out_MCR(p, mcr);
 }
 
-- 
2.17.0

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 8/8] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set.
  2018-06-01 12:40 [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw Giulio Benetti
                   ` (6 preceding siblings ...)
  2018-06-01 12:40 ` [PATCH 7/8] serial: 8250: Make em485_rts_after_send() set mctrl according to rts state Giulio Benetti
@ 2018-06-01 12:40 ` Giulio Benetti
  2018-06-04 10:12 ` [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw Andy Shevchenko
  8 siblings, 0 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-01 12:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, Andy Shevchenko, Kees Cook, Matthias Brugger,
	Allen Pais, Sean Young, Ed Blake, Stefan Potyra, Philipp Zabel,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Rafael Gago, Joel Stanley, Sean Wang, linux-serial, linux-kernel,
	Giulio Benetti

If rs485 is enabled and RTS_AFTER_SEND is set on startup need to keep
TIOCM_RTS asserted to keep rs485 transceiver in RX when idle.

Check if rs485 is on and RTS_AFTER_SEND is set and mask port->mctrl with
TIOCM_RTS too and not only TIOCM_DTR.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/serial_core.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 0466f9f08a91..06d9441f6d20 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2288,6 +2288,16 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 
 	if (port->type != PORT_UNKNOWN) {
 		unsigned long flags;
+		int rs485_on = port->rs485_config &&
+			(port->rs485.flags & SER_RS485_ENABLED);
+		int RTS_after_send = !!(port->rs485.flags &
+				SER_RS485_RTS_AFTER_SEND);
+		int mctrl;
+
+		if (rs485_on && RTS_after_send)
+			mctrl = port->mctrl & (TIOCM_DTR | TIOCM_RTS);
+		else
+			mctrl = port->mctrl & TIOCM_DTR;
 
 		uart_report_port(drv, port);
 
@@ -2300,7 +2310,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 		 * We probably don't need a spinlock around this, but
 		 */
 		spin_lock_irqsave(&port->lock, flags);
-		port->ops->set_mctrl(port, port->mctrl & TIOCM_DTR);
+		port->ops->set_mctrl(port, mctrl);
 		spin_unlock_irqrestore(&port->lock, flags);
 
 		/*
-- 
2.17.0

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw.
  2018-06-01 12:40 [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw Giulio Benetti
                   ` (7 preceding siblings ...)
  2018-06-01 12:40 ` [PATCH 8/8] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set Giulio Benetti
@ 2018-06-04 10:12 ` Andy Shevchenko
  2018-06-04 10:34   ` Matwey V. Kornilov
  8 siblings, 1 reply; 50+ messages in thread
From: Andy Shevchenko @ 2018-06-04 10:12 UTC (permalink / raw)
  To: Giulio Benetti, Greg Kroah-Hartman, Matwey V.Kornilov
  Cc: Jiri Slaby, Kees Cook, Matthias Brugger, Allen Pais, Sean Young,
	Ed Blake, Stefan Potyra, Philipp Zabel, Joshua Scott, Vignesh R,
	Rolf Evers-Fischer, Aaron Sierra, Rafael Gago, Joel Stanley,
	Sean Wang, linux-serial, linux-kernel

On Fri, 2018-06-01 at 14:40 +0200, Giulio Benetti wrote:
> Need to handle rs485 with 8250_dw port.
> 
> Use existing em485 emulation layer for 8250 taking care to fix some
> bug
> and taking care especially of RTS_AFTER_SEND case.
> 

I don't see in Cc list author of rs485 code, who might be interested in
this.

So, added him here.

> Giulio Benetti (8):
>   serial: 8250_dw: add em485 support
>   serial: 8250_dw: allow enable rs485 at boot time
>   serial: 8250: Copy em485 from port to real port.
>   serial: 8250: Handle case port doesn't have TEMT interrupt using
>     em485.
>   serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and
>     RTS_AFTER_SEND
>   serial: 8250: Copy mctrl when register port.
>   serial: 8250: Make em485_rts_after_send() set mctrl according to rts
>     state.
>   serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and
>     RTS_AFTER_SEND set.
> 
>  drivers/tty/serial/8250/8250.h      |  2 +-
>  drivers/tty/serial/8250/8250_core.c |  2 ++
>  drivers/tty/serial/8250/8250_dw.c   | 41
> ++++++++++++++++++++++++++++-
>  drivers/tty/serial/8250/8250_omap.c |  2 +-
>  drivers/tty/serial/8250/8250_port.c | 33 ++++++++++++++++-------
>  drivers/tty/serial/serial_core.c    | 12 ++++++++-
>  include/linux/serial_8250.h         |  1 +
>  7 files changed, 79 insertions(+), 14 deletions(-)
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 3/8] serial: 8250: Copy em485 from port to real port.
  2018-06-01 12:40 ` [PATCH 3/8] serial: 8250: Copy em485 from port to real port Giulio Benetti
@ 2018-06-04 10:13   ` Andy Shevchenko
  2018-06-04 10:52     ` Giulio Benetti
  0 siblings, 1 reply; 50+ messages in thread
From: Andy Shevchenko @ 2018-06-04 10:13 UTC (permalink / raw)
  To: Giulio Benetti, Greg Kroah-Hartman
  Cc: Jiri Slaby, Kees Cook, Matthias Brugger, Allen Pais, Sean Young,
	Ed Blake, Stefan Potyra, Philipp Zabel, Joshua Scott, Vignesh R,
	Rolf Evers-Fischer, Aaron Sierra, Rafael Gago, Joel Stanley,
	Sean Wang, linux-serial, linux-kernel

On Fri, 2018-06-01 at 14:40 +0200, Giulio Benetti wrote:
> em485 gets lost during serial8250_register_8250_port().
> 
> Copy em485 to final uart port.

Fixes better to go first.
I think you need to reorder the series.

> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>  drivers/tty/serial/8250/8250_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c
> b/drivers/tty/serial/8250/8250_core.c
> index 9342fc2ee7df..c8c2b260c681 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -994,6 +994,7 @@ int serial8250_register_8250_port(struct
> uart_8250_port *up)
>  		uart->port.rs485_config	= up-
> >port.rs485_config;
>  		uart->port.rs485	= up->port.rs485;
>  		uart->dma		= up->dma;
> +		uart->em485		= up->em485;
>  
>  		/* Take tx_loadsz from fifosize if it wasn't set
> separately */
>  		if (uart->port.fifosize && !uart->tx_loadsz)

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.
  2018-06-01 12:40 ` [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485 Giulio Benetti
@ 2018-06-04 10:17   ` Andy Shevchenko
  2018-06-04 10:50     ` Giulio Benetti
  2018-06-04 17:40   ` Matwey V. Kornilov
  1 sibling, 1 reply; 50+ messages in thread
From: Andy Shevchenko @ 2018-06-04 10:17 UTC (permalink / raw)
  To: Giulio Benetti, Greg Kroah-Hartman
  Cc: Jiri Slaby, Kees Cook, Matthias Brugger, Allen Pais, Sean Young,
	Ed Blake, Stefan Potyra, Philipp Zabel, Joshua Scott, Vignesh R,
	Rolf Evers-Fischer, Aaron Sierra, Rafael Gago, Joel Stanley,
	Sean Wang, linux-serial, linux-kernel

On Fri, 2018-06-01 at 14:40 +0200, Giulio Benetti wrote:
> Some 8250 ports only have TEMT interrupt, so current implementation
> can't work for ports without it. The only chance to make it work is to
> loop-read on LSR register.
> 
> With NO TEMT interrupt check if both TEMT and THRE are set looping on
> LSR register.

> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c

> -		int ret = serial8250_em485_init(up);
> +		int ret = serial8250_em485_init(up, false);

Is true for all possible DW configured types? Or it's your particular
case?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw.
  2018-06-04 10:12 ` [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw Andy Shevchenko
@ 2018-06-04 10:34   ` Matwey V. Kornilov
  2018-06-04 10:42     ` Giulio Benetti
  0 siblings, 1 reply; 50+ messages in thread
From: Matwey V. Kornilov @ 2018-06-04 10:34 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Giulio Benetti, Greg Kroah-Hartman, Jiri Slaby, Kees Cook,
	Matthias Brugger, Allen Pais, Sean Young, Ed Blake,
	Stefan Potyra, Philipp Zabel, Joshua Scott, Vignesh R,
	Rolf Evers-Fischer, Aaron Sierra, Rafael Gago, Joel Stanley,
	Sean Wang, linux-serial, linux-kernel

2018-06-04 13:12 GMT+03:00 Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> On Fri, 2018-06-01 at 14:40 +0200, Giulio Benetti wrote:
>> Need to handle rs485 with 8250_dw port.
>>
>> Use existing em485 emulation layer for 8250 taking care to fix some
>> bug
>> and taking care especially of RTS_AFTER_SEND case.
>>
>
> I don't see in Cc list author of rs485 code, who might be interested in
> this.
>
> So, added him here.
>

Hi, Andy

Nice to see you there. I will look through the code later.
I always thought that DW has its own hardware rs485 support.

>> Giulio Benetti (8):
>>   serial: 8250_dw: add em485 support
>>   serial: 8250_dw: allow enable rs485 at boot time
>>   serial: 8250: Copy em485 from port to real port.
>>   serial: 8250: Handle case port doesn't have TEMT interrupt using
>>     em485.
>>   serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and
>>     RTS_AFTER_SEND
>>   serial: 8250: Copy mctrl when register port.
>>   serial: 8250: Make em485_rts_after_send() set mctrl according to rts
>>     state.
>>   serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and
>>     RTS_AFTER_SEND set.
>>
>>  drivers/tty/serial/8250/8250.h      |  2 +-
>>  drivers/tty/serial/8250/8250_core.c |  2 ++
>>  drivers/tty/serial/8250/8250_dw.c   | 41
>> ++++++++++++++++++++++++++++-
>>  drivers/tty/serial/8250/8250_omap.c |  2 +-
>>  drivers/tty/serial/8250/8250_port.c | 33 ++++++++++++++++-------
>>  drivers/tty/serial/serial_core.c    | 12 ++++++++-
>>  include/linux/serial_8250.h         |  1 +
>>  7 files changed, 79 insertions(+), 14 deletions(-)
>>
>
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy
>



-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw.
  2018-06-04 10:34   ` Matwey V. Kornilov
@ 2018-06-04 10:42     ` Giulio Benetti
  2018-06-04 11:44       ` Andy Shevchenko
  0 siblings, 1 reply; 50+ messages in thread
From: Giulio Benetti @ 2018-06-04 10:42 UTC (permalink / raw)
  To: Matwey V. Kornilov, Andy Shevchenko
  Cc: Greg Kroah-Hartman, Jiri Slaby, Kees Cook, Matthias Brugger,
	Allen Pais, Sean Young, Ed Blake, Stefan Potyra, Philipp Zabel,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Rafael Gago, Joel Stanley, Sean Wang, linux-serial, linux-kernel

Hi everybody,

Il 04/06/2018 12:34, Matwey V. Kornilov ha scritto:
> 2018-06-04 13:12 GMT+03:00 Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
>> On Fri, 2018-06-01 at 14:40 +0200, Giulio Benetti wrote:
>>> Need to handle rs485 with 8250_dw port.
>>>
>>> Use existing em485 emulation layer for 8250 taking care to fix some
>>> bug
>>> and taking care especially of RTS_AFTER_SEND case.
>>>
>>
>> I don't see in Cc list author of rs485 code, who might be interested in
>> this.
>>
>> So, added him here.

Thanks Andy for adding in Cc an reviewing.
Dumb question: how did you find him?
I thought he was between people on get_maintainer.pl output.

>>
> 
> Hi, Andy
> 
> Nice to see you there. I will look through the code later.
> I always thought that DW has its own hardware rs485 support.

Hi Matwey,
I've checked on Synopsis Designware 8250 datasheet and it's not supported.
Here is datasheet I went through:
https://linux-sunxi.org/images/d/d2/Dw_apb_uart_db.pdf

On IER register there is not TEMT bit or something like that used in 
8250_omap.

Am I right?

Thanks everybody and
best regards

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

> 
>>> Giulio Benetti (8):
>>>    serial: 8250_dw: add em485 support
>>>    serial: 8250_dw: allow enable rs485 at boot time
>>>    serial: 8250: Copy em485 from port to real port.
>>>    serial: 8250: Handle case port doesn't have TEMT interrupt using
>>>      em485.
>>>    serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and
>>>      RTS_AFTER_SEND
>>>    serial: 8250: Copy mctrl when register port.
>>>    serial: 8250: Make em485_rts_after_send() set mctrl according to rts
>>>      state.
>>>    serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and
>>>      RTS_AFTER_SEND set.
>>>
>>>   drivers/tty/serial/8250/8250.h      |  2 +-
>>>   drivers/tty/serial/8250/8250_core.c |  2 ++
>>>   drivers/tty/serial/8250/8250_dw.c   | 41
>>> ++++++++++++++++++++++++++++-
>>>   drivers/tty/serial/8250/8250_omap.c |  2 +-
>>>   drivers/tty/serial/8250/8250_port.c | 33 ++++++++++++++++-------
>>>   drivers/tty/serial/serial_core.c    | 12 ++++++++-
>>>   include/linux/serial_8250.h         |  1 +
>>>   7 files changed, 79 insertions(+), 14 deletions(-)
>>>
>>
>> --
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Intel Finland Oy
>>
> 
> 
> 

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.
  2018-06-04 10:17   ` Andy Shevchenko
@ 2018-06-04 10:50     ` Giulio Benetti
  2018-06-04 11:38       ` Andy Shevchenko
  0 siblings, 1 reply; 50+ messages in thread
From: Giulio Benetti @ 2018-06-04 10:50 UTC (permalink / raw)
  To: Andy Shevchenko, Greg Kroah-Hartman
  Cc: Jiri Slaby, Kees Cook, Matthias Brugger, Allen Pais, Sean Young,
	Ed Blake, Stefan Potyra, Philipp Zabel, Joshua Scott, Vignesh R,
	Rolf Evers-Fischer, Aaron Sierra, Rafael Gago, Joel Stanley,
	Sean Wang, linux-serial, linux-kernel

Hi,

Il 04/06/2018 12:17, Andy Shevchenko ha scritto:
> On Fri, 2018-06-01 at 14:40 +0200, Giulio Benetti wrote:
>> Some 8250 ports only have TEMT interrupt, so current implementation
>> can't work for ports without it. The only chance to make it work is to
>> loop-read on LSR register.
>>
>> With NO TEMT interrupt check if both TEMT and THRE are set looping on
>> LSR register.
> 
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
> 
>> -		int ret = serial8250_em485_init(up);
>> +		int ret = serial8250_em485_init(up, false);
> 
> Is true for all possible DW configured types? Or it's your particular
> case?
> 

I've checked on Synopsis Designware 8250 datasheet and it's not supported.
Here is datasheet I went through:
https://linux-sunxi.org/images/d/d2/Dw_apb_uart_db.pdf

There seems not to be TEMT interrupt, I use it under sunxi SoC and on 
their datasheet(A20 for example), they don't report that interrupt too.
So it seems to be valid for all DW configured types, anyway I don't know 
how many IP reviews there could be of that peripheral.
I've tried to subscribe at Synopsis to obtain latest Datasheet but it 
ask me an active ID I don't have.

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 3/8] serial: 8250: Copy em485 from port to real port.
  2018-06-04 10:13   ` Andy Shevchenko
@ 2018-06-04 10:52     ` Giulio Benetti
  0 siblings, 0 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-04 10:52 UTC (permalink / raw)
  To: Andy Shevchenko, Greg Kroah-Hartman
  Cc: Jiri Slaby, Kees Cook, Matthias Brugger, Allen Pais, Sean Young,
	Ed Blake, Stefan Potyra, Philipp Zabel, Joshua Scott, Vignesh R,
	Rolf Evers-Fischer, Aaron Sierra, Rafael Gago, Joel Stanley,
	Sean Wang, linux-serial, linux-kernel

Hi,

Il 04/06/2018 12:13, Andy Shevchenko ha scritto:
> On Fri, 2018-06-01 at 14:40 +0200, Giulio Benetti wrote:
>> em485 gets lost during serial8250_register_8250_port().
>>
>> Copy em485 to final uart port.
> 
> Fixes better to go first.
> I think you need to reorder the series.

Ok, thanks.
So after re-ording and clarifying TEMT interrupt, can I send v2 patchset?

Thanks

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.
  2018-06-04 10:50     ` Giulio Benetti
@ 2018-06-04 11:38       ` Andy Shevchenko
  2018-06-04 11:50         ` Giulio Benetti
  0 siblings, 1 reply; 50+ messages in thread
From: Andy Shevchenko @ 2018-06-04 11:38 UTC (permalink / raw)
  To: Giulio Benetti, Greg Kroah-Hartman
  Cc: Jiri Slaby, Kees Cook, Matthias Brugger, Allen Pais, Sean Young,
	Ed Blake, Stefan Potyra, Philipp Zabel, Joshua Scott, Vignesh R,
	Rolf Evers-Fischer, Aaron Sierra, Rafael Gago, Joel Stanley,
	Sean Wang, linux-serial, linux-kernel

On Mon, 2018-06-04 at 12:50 +0200, Giulio Benetti wrote:
> Hi,
> 
> Il 04/06/2018 12:17, Andy Shevchenko ha scritto:
> > On Fri, 2018-06-01 at 14:40 +0200, Giulio Benetti wrote:
> > > Some 8250 ports only have TEMT interrupt, so current
> > > implementation
> > > can't work for ports without it. The only chance to make it work
> > > is to
> > > loop-read on LSR register.
> > > 
> > > With NO TEMT interrupt check if both TEMT and THRE are set looping
> > > on
> > > LSR register.
> > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > -		int ret = serial8250_em485_init(up);
> > > +		int ret = serial8250_em485_init(up, false);
> > 
> > Is true for all possible DW configured types? Or it's your
> > particular
> > case?
> > 
> 
> I've checked on Synopsis Designware 8250 datasheet and it's not
> supported.
> Here is datasheet I went through:
> https://linux-sunxi.org/images/d/d2/Dw_apb_uart_db.pdf
> 
> There seems not to be TEMT interrupt, I use it under sunxi SoC and on 
> their datasheet(A20 for example), they don't report that interrupt
> too.
> So it seems to be valid for all DW configured types, anyway I don't
> know 
> how many IP reviews there could be of that peripheral.

This is an excerpt from the document you referred to:

--- 8< --- 8< ---

6 TEMT R Transmitter Empty bit. If in FIFO mode (FIFO_MODE != NONE) and
FIFOs enabled (FCR[0] set to one), this bit is set whenever the
Transmitter Shift Register and the FIFO are both empty. If in non-FIFO
mode or FIFOs are disabled, this bit is set whenever the Transmitter
Holding Register and the Transmitter Shift Register are both empty.

Reset Value: 0x1

--- 8< --- 8< ---


If I'm reading this correctly the support is there. Or otherwise, care
to point exact paragraph needs to be read and checked?

> I've tried to subscribe at Synopsis to obtain latest Datasheet but it 
> ask me an active ID I don't have.
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw.
  2018-06-04 10:42     ` Giulio Benetti
@ 2018-06-04 11:44       ` Andy Shevchenko
  2018-06-04 14:58         ` Giulio Benetti
  0 siblings, 1 reply; 50+ messages in thread
From: Andy Shevchenko @ 2018-06-04 11:44 UTC (permalink / raw)
  To: Giulio Benetti, Matwey V. Kornilov
  Cc: Greg Kroah-Hartman, Jiri Slaby, Kees Cook, Matthias Brugger,
	Allen Pais, Sean Young, Ed Blake, Stefan Potyra, Philipp Zabel,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Rafael Gago, Joel Stanley, Sean Wang, linux-serial, linux-kernel

On Mon, 2018-06-04 at 12:42 +0200, Giulio Benetti wrote:

> > > I don't see in Cc list author of rs485 code, who might be
> > > interested in
> > > this.
> > > 
> > > So, added him here.
> 
> Thanks Andy for adding in Cc an reviewing.
> Dumb question: how did you find him?

I'm just following mailing lists somehow.
Though you always can do `git annotate` to see who lately did change
what.

> I thought he was between people on get_maintainer.pl output.

Hmm... probably. Never checked myself for this particular case.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.
  2018-06-04 11:38       ` Andy Shevchenko
@ 2018-06-04 11:50         ` Giulio Benetti
  2018-06-04 12:26           ` Andy Shevchenko
  0 siblings, 1 reply; 50+ messages in thread
From: Giulio Benetti @ 2018-06-04 11:50 UTC (permalink / raw)
  To: Andy Shevchenko, Greg Kroah-Hartman
  Cc: Jiri Slaby, Kees Cook, Matthias Brugger, Allen Pais, Sean Young,
	Ed Blake, Stefan Potyra, Philipp Zabel, Joshua Scott, Vignesh R,
	Rolf Evers-Fischer, Aaron Sierra, Rafael Gago, Joel Stanley,
	Sean Wang, linux-serial, linux-kernel

Hi,

Il 04/06/2018 13:38, Andy Shevchenko ha scritto:
> On Mon, 2018-06-04 at 12:50 +0200, Giulio Benetti wrote:
>> Hi,
>>
>> Il 04/06/2018 12:17, Andy Shevchenko ha scritto:
>>> On Fri, 2018-06-01 at 14:40 +0200, Giulio Benetti wrote:
>>>> Some 8250 ports only have TEMT interrupt, so current
>>>> implementation
>>>> can't work for ports without it. The only chance to make it work
>>>> is to
>>>> loop-read on LSR register.
>>>>
>>>> With NO TEMT interrupt check if both TEMT and THRE are set looping
>>>> on
>>>> LSR register.
>>>> --- a/drivers/tty/serial/8250/8250_dw.c
>>>> +++ b/drivers/tty/serial/8250/8250_dw.c
>>>> -		int ret = serial8250_em485_init(up);
>>>> +		int ret = serial8250_em485_init(up, false);
>>>
>>> Is true for all possible DW configured types? Or it's your
>>> particular
>>> case?
>>>
>>
>> I've checked on Synopsis Designware 8250 datasheet and it's not
>> supported.
>> Here is datasheet I went through:
>> https://linux-sunxi.org/images/d/d2/Dw_apb_uart_db.pdf
>>
>> There seems not to be TEMT interrupt, I use it under sunxi SoC and on
>> their datasheet(A20 for example), they don't report that interrupt
>> too.
>> So it seems to be valid for all DW configured types, anyway I don't
>> know
>> how many IP reviews there could be of that peripheral.
> 
> This is an excerpt from the document you referred to:
> 
> --- 8< --- 8< ---
> 
> 6 TEMT R Transmitter Empty bit. If in FIFO mode (FIFO_MODE != NONE) and
> FIFOs enabled (FCR[0] set to one), this bit is set whenever the
> Transmitter Shift Register and the FIFO are both empty. If in non-FIFO
> mode or FIFOs are disabled, this bit is set whenever the Transmitter
> Holding Register and the Transmitter Shift Register are both empty.
> 
> Reset Value: 0x1
> 
> --- 8< --- 8< ---
> 
> 
> If I'm reading this correctly the support is there. Or otherwise, care
> to point exact paragraph needs to be read and checked?

In the beginning I thought the same as you but
unfortunately LSR is only a status register and IER doesn't have 
corresponding TEMT bit to enable an interrupt on TEMT triggering.
On OMAP instead there is a specific interrupt bound to TEMT LSR flag.
And THRE interrupt is not enough because shift register won't be empty 
when it triggers, so you would loose some bit of last byte to be 
transmitted.

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.
  2018-06-04 11:50         ` Giulio Benetti
@ 2018-06-04 12:26           ` Andy Shevchenko
  0 siblings, 0 replies; 50+ messages in thread
From: Andy Shevchenko @ 2018-06-04 12:26 UTC (permalink / raw)
  To: Giulio Benetti, Greg Kroah-Hartman, Matwey V.Kornilov
  Cc: Jiri Slaby, Kees Cook, Matthias Brugger, Allen Pais, Sean Young,
	Ed Blake, Stefan Potyra, Philipp Zabel, Joshua Scott, Vignesh R,
	Rolf Evers-Fischer, Aaron Sierra, Rafael Gago, Joel Stanley,
	Sean Wang, linux-serial, linux-kernel

On Mon, 2018-06-04 at 13:50 +0200, Giulio Benetti wrote:
> Hi,
> 
> Il 04/06/2018 13:38, Andy Shevchenko ha scritto:
> > On Mon, 2018-06-04 at 12:50 +0200, Giulio Benetti wrote:
> > > Hi,
> > > 
> > > Il 04/06/2018 12:17, Andy Shevchenko ha scritto:
> > > > On Fri, 2018-06-01 at 14:40 +0200, Giulio Benetti wrote:
> > > > > Some 8250 ports only have TEMT interrupt, so current
> > > > > implementation
> > > > > can't work for ports without it. The only chance to make it
> > > > > work
> > > > > is to
> > > > > loop-read on LSR register.
> > > > > 
> > > > > With NO TEMT interrupt check if both TEMT and THRE are set
> > > > > looping
> > > > > on
> > > > > LSR register.
> > > > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > > > -		int ret = serial8250_em485_init(up);
> > > > > +		int ret = serial8250_em485_init(up, false);
> > > > 
> > > > Is true for all possible DW configured types? Or it's your
> > > > particular
> > > > case?
> > > > 
> > > 
> > > I've checked on Synopsis Designware 8250 datasheet and it's not
> > > supported.
> > > Here is datasheet I went through:
> > > https://linux-sunxi.org/images/d/d2/Dw_apb_uart_db.pdf
> > > 
> > > There seems not to be TEMT interrupt, I use it under sunxi SoC and
> > > on
> > > their datasheet(A20 for example), they don't report that interrupt
> > > too.
> > > So it seems to be valid for all DW configured types, anyway I
> > > don't
> > > know
> > > how many IP reviews there could be of that peripheral.
> > 
> > This is an excerpt from the document you referred to:
> > 
> > --- 8< --- 8< ---
> > 
> > 6 TEMT R Transmitter Empty bit. If in FIFO mode (FIFO_MODE != NONE)
> > and
> > FIFOs enabled (FCR[0] set to one), this bit is set whenever the
> > Transmitter Shift Register and the FIFO are both empty. If in non-
> > FIFO
> > mode or FIFOs are disabled, this bit is set whenever the Transmitter
> > Holding Register and the Transmitter Shift Register are both empty.
> > 
> > Reset Value: 0x1
> > 
> > --- 8< --- 8< ---
> > 
> > 
> > If I'm reading this correctly the support is there. Or otherwise,
> > care
> > to point exact paragraph needs to be read and checked?
> 
> In the beginning I thought the same as you but
> unfortunately LSR is only a status register and IER doesn't have 
> corresponding TEMT bit to enable an interrupt on TEMT triggering.
> On OMAP instead there is a specific interrupt bound to TEMT LSR flag.
> And THRE interrupt is not enough because shift register won't be
> empty 
> when it triggers, so you would loose some bit of last byte to be 
> transmitted.

Hmm... Okay, it's something you and Matwey better to discuss.

P.S. Latest version of document I have does describe RS485 HW supported
mode. I don't know if it was added recently to the IP itself, or just
missed documentation. That's what you need to clarify with Synopsys.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw.
  2018-06-04 11:44       ` Andy Shevchenko
@ 2018-06-04 14:58         ` Giulio Benetti
  0 siblings, 0 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-04 14:58 UTC (permalink / raw)
  To: Andy Shevchenko, Matwey V. Kornilov
  Cc: Greg Kroah-Hartman, Jiri Slaby, Kees Cook, Matthias Brugger,
	Allen Pais, Sean Young, Ed Blake, Stefan Potyra, Philipp Zabel,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Rafael Gago, Joel Stanley, Sean Wang, linux-serial, linux-kernel


Il 04/06/2018 13:44, Andy Shevchenko ha scritto:
> On Mon, 2018-06-04 at 12:42 +0200, Giulio Benetti wrote:
> 
>>>> I don't see in Cc list author of rs485 code, who might be
>>>> interested in
>>>> this.
>>>>
>>>> So, added him here.
>>
>> Thanks Andy for adding in Cc an reviewing.
>> Dumb question: how did you find him?
> 
> I'm just following mailing lists somehow.
> Though you always can do `git annotate` to see who lately did change
> what.
> 
>> I thought he was between people on get_maintainer.pl output.
> 
> Hmm... probably. Never checked myself for this particular case.
> 

Ok, thank you.

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.
  2018-06-01 12:40 ` [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485 Giulio Benetti
  2018-06-04 10:17   ` Andy Shevchenko
@ 2018-06-04 17:40   ` Matwey V. Kornilov
  2018-06-04 18:50     ` Giulio Benetti
  1 sibling, 1 reply; 50+ messages in thread
From: Matwey V. Kornilov @ 2018-06-04 17:40 UTC (permalink / raw)
  To: Giulio Benetti, Greg Kroah-Hartman
  Cc: Jiri Slaby, Andy Shevchenko, Kees Cook, Matthias Brugger,
	Allen Pais, Sean Young, Ed Blake, Stefan Potyra, Philipp Zabel,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Rafael Gago, Joel Stanley, Sean Wang, linux-serial, linux-kernel

01.06.2018 15:40, Giulio Benetti пишет:
> Some 8250 ports only have TEMT interrupt, so current implementation
> can't work for ports without it. The only chance to make it work is to
> loop-read on LSR register.
> 
> With NO TEMT interrupt check if both TEMT and THRE are set looping on
> LSR register.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>  drivers/tty/serial/8250/8250.h      |  2 +-
>  drivers/tty/serial/8250/8250_dw.c   |  2 +-
>  drivers/tty/serial/8250/8250_omap.c |  2 +-
>  drivers/tty/serial/8250/8250_port.c | 26 ++++++++++++++++++--------
>  include/linux/serial_8250.h         |  1 +
>  5 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index ebfb0bd5bef5..5c6ae5f69432 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -136,7 +136,7 @@ void serial8250_rpm_put(struct uart_8250_port *p);
>  void serial8250_rpm_get_tx(struct uart_8250_port *p);
>  void serial8250_rpm_put_tx(struct uart_8250_port *p);
>  
> -int serial8250_em485_init(struct uart_8250_port *p);
> +int serial8250_em485_init(struct uart_8250_port *p, bool has_temt_isr);
>  void serial8250_em485_destroy(struct uart_8250_port *p);
>  
>  static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 0f8b4da03d4e..888280ff5451 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -330,7 +330,7 @@ static int dw8250_rs485_config(struct uart_port *p,
>  	 * are idempotent
>  	 */
>  	if (rs485->flags & SER_RS485_ENABLED) {
> -		int ret = serial8250_em485_init(up);
> +		int ret = serial8250_em485_init(up, false);
>  
>  		if (ret) {
>  			rs485->flags &= ~SER_RS485_ENABLED;
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 624b501fd253..ab483c8b30c8 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -725,7 +725,7 @@ static int omap_8250_rs485_config(struct uart_port *port,
>  	 * are idempotent
>  	 */
>  	if (rs485->flags & SER_RS485_ENABLED) {
> -		int ret = serial8250_em485_init(up);
> +		int ret = serial8250_em485_init(up, true);
>  
>  		if (ret) {
>  			rs485->flags &= ~SER_RS485_ENABLED;
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 95833cbc4338..e14badbbf181 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -599,15 +599,16 @@ EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>  /**
>   *	serial8250_em485_init() - put uart_8250_port into rs485 emulating
>   *	@p:	uart_8250_port port instance
> + *	@p:	bool specify if 8250 port has TEMT interrupt or not
>   *
>   *	The function is used to start rs485 software emulating on the
>   *	&struct uart_8250_port* @p. Namely, RTS is switched before/after
>   *	transmission. The function is idempotent, so it is safe to call it
>   *	multiple times.
>   *
> - *	The caller MUST enable interrupt on empty shift register before
> - *	calling serial8250_em485_init(). This interrupt is not a part of
> - *	8250 standard, but implementation defined.
> + *	If has_temt_isr is passed as true, the caller MUST enable interrupt
> + *	on empty shift register before calling serial8250_em485_init().
> + *	This interrupt is not a part of	8250 standard, but implementation defined.
>   *
>   *	The function is supposed to be called from .rs485_config callback
>   *	or from any other callback protected with p->port.lock spinlock.
> @@ -616,7 +617,7 @@ EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>   *
>   *	Return 0 - success, -errno - otherwise
>   */
> -int serial8250_em485_init(struct uart_8250_port *p)
> +int serial8250_em485_init(struct uart_8250_port *p, bool has_temt_isr)
>  {
>  	if (p->em485)
>  		return 0;
> @@ -633,6 +634,7 @@ int serial8250_em485_init(struct uart_8250_port *p)
>  	p->em485->start_tx_timer.function = &serial8250_em485_handle_start_tx;
>  	p->em485->port = p;
>  	p->em485->active_timer = NULL;
> +	p->em485->has_temt_isr = has_temt_isr;
>  	serial8250_em485_rts_after_send(p);
>  
>  	return 0;
> @@ -1517,11 +1519,19 @@ static inline void __stop_tx(struct uart_8250_port *p)
>  		/*
>  		 * To provide required timeing 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.
> +		 * shift register are empty. If 8250 port supports it,
> +		 * it is for device driver to enable interrupt on TEMT.
> +		 * Otherwise must loop-read until TEMT and THRE flags are set.
>  		 */
> -		if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
> -			return;
> +		if (em485->has_temt_isr) {
> +			if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
> +				return;
> +		} else {
> +			while ((lsr & BOTH_EMPTY) != BOTH_EMPTY) {
> +				lsr = serial_in(p, UART_LSR);
> +				cpu_relax();
> +			}

What happens if TEMP never be empty after interruption occurring?

> +		}
>  
>  		em485->active_timer = NULL;
>  
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index a27ef5f56431..9b13cf59726b 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -83,6 +83,7 @@ struct uart_8250_em485 {
>  	struct hrtimer		start_tx_timer; /* "rs485 start tx" timer */
>  	struct hrtimer		stop_tx_timer;  /* "rs485 stop tx" timer */
>  	struct hrtimer		*active_timer;  /* pointer to active timer */
> +	bool			has_temt_isr;	/* say if 8250 has TEMT interrupt or no */
>  	struct uart_8250_port	*port;          /* for hrtimer callbacks */
>  };
>  
> 

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.
  2018-06-04 17:40   ` Matwey V. Kornilov
@ 2018-06-04 18:50     ` Giulio Benetti
  2018-06-05 10:51       ` Matwey V. Kornilov
  0 siblings, 1 reply; 50+ messages in thread
From: Giulio Benetti @ 2018-06-04 18:50 UTC (permalink / raw)
  To: Matwey V. Kornilov, Greg Kroah-Hartman
  Cc: Jiri Slaby, Andy Shevchenko, Kees Cook, Matthias Brugger,
	Allen Pais, Sean Young, Ed Blake, Stefan Potyra, Philipp Zabel,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Rafael Gago, Joel Stanley, Sean Wang, linux-serial, linux-kernel

Il 04/06/2018 19:40, Matwey V. Kornilov ha scritto:
> 01.06.2018 15:40, Giulio Benetti пишет:
>> Some 8250 ports only have TEMT interrupt, so current implementation
>> can't work for ports without it. The only chance to make it work is to
>> loop-read on LSR register.
>>
>> With NO TEMT interrupt check if both TEMT and THRE are set looping on
>> LSR register.
>>
>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>> ---
>>   drivers/tty/serial/8250/8250.h      |  2 +-
>>   drivers/tty/serial/8250/8250_dw.c   |  2 +-
>>   drivers/tty/serial/8250/8250_omap.c |  2 +-
>>   drivers/tty/serial/8250/8250_port.c | 26 ++++++++++++++++++--------
>>   include/linux/serial_8250.h         |  1 +
>>   5 files changed, 22 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>> index ebfb0bd5bef5..5c6ae5f69432 100644
>> --- a/drivers/tty/serial/8250/8250.h
>> +++ b/drivers/tty/serial/8250/8250.h
>> @@ -136,7 +136,7 @@ void serial8250_rpm_put(struct uart_8250_port *p);
>>   void serial8250_rpm_get_tx(struct uart_8250_port *p);
>>   void serial8250_rpm_put_tx(struct uart_8250_port *p);
>>   
>> -int serial8250_em485_init(struct uart_8250_port *p);
>> +int serial8250_em485_init(struct uart_8250_port *p, bool has_temt_isr);
>>   void serial8250_em485_destroy(struct uart_8250_port *p);
>>   
>>   static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
>> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
>> index 0f8b4da03d4e..888280ff5451 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
>> @@ -330,7 +330,7 @@ static int dw8250_rs485_config(struct uart_port *p,
>>   	 * are idempotent
>>   	 */
>>   	if (rs485->flags & SER_RS485_ENABLED) {
>> -		int ret = serial8250_em485_init(up);
>> +		int ret = serial8250_em485_init(up, false);
>>   
>>   		if (ret) {
>>   			rs485->flags &= ~SER_RS485_ENABLED;
>> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
>> index 624b501fd253..ab483c8b30c8 100644
>> --- a/drivers/tty/serial/8250/8250_omap.c
>> +++ b/drivers/tty/serial/8250/8250_omap.c
>> @@ -725,7 +725,7 @@ static int omap_8250_rs485_config(struct uart_port *port,
>>   	 * are idempotent
>>   	 */
>>   	if (rs485->flags & SER_RS485_ENABLED) {
>> -		int ret = serial8250_em485_init(up);
>> +		int ret = serial8250_em485_init(up, true);
>>   
>>   		if (ret) {
>>   			rs485->flags &= ~SER_RS485_ENABLED;
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 95833cbc4338..e14badbbf181 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -599,15 +599,16 @@ EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>   /**
>>    *	serial8250_em485_init() - put uart_8250_port into rs485 emulating
>>    *	@p:	uart_8250_port port instance
>> + *	@p:	bool specify if 8250 port has TEMT interrupt or not
>>    *
>>    *	The function is used to start rs485 software emulating on the
>>    *	&struct uart_8250_port* @p. Namely, RTS is switched before/after
>>    *	transmission. The function is idempotent, so it is safe to call it
>>    *	multiple times.
>>    *
>> - *	The caller MUST enable interrupt on empty shift register before
>> - *	calling serial8250_em485_init(). This interrupt is not a part of
>> - *	8250 standard, but implementation defined.
>> + *	If has_temt_isr is passed as true, the caller MUST enable interrupt
>> + *	on empty shift register before calling serial8250_em485_init().
>> + *	This interrupt is not a part of	8250 standard, but implementation defined.
>>    *
>>    *	The function is supposed to be called from .rs485_config callback
>>    *	or from any other callback protected with p->port.lock spinlock.
>> @@ -616,7 +617,7 @@ EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>    *
>>    *	Return 0 - success, -errno - otherwise
>>    */
>> -int serial8250_em485_init(struct uart_8250_port *p)
>> +int serial8250_em485_init(struct uart_8250_port *p, bool has_temt_isr)
>>   {
>>   	if (p->em485)
>>   		return 0;
>> @@ -633,6 +634,7 @@ int serial8250_em485_init(struct uart_8250_port *p)
>>   	p->em485->start_tx_timer.function = &serial8250_em485_handle_start_tx;
>>   	p->em485->port = p;
>>   	p->em485->active_timer = NULL;
>> +	p->em485->has_temt_isr = has_temt_isr;
>>   	serial8250_em485_rts_after_send(p);
>>   
>>   	return 0;
>> @@ -1517,11 +1519,19 @@ static inline void __stop_tx(struct uart_8250_port *p)
>>   		/*
>>   		 * To provide required timeing 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.
>> +		 * shift register are empty. If 8250 port supports it,
>> +		 * it is for device driver to enable interrupt on TEMT.
>> +		 * Otherwise must loop-read until TEMT and THRE flags are set.
>>   		 */
>> -		if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>> -			return;
>> +		if (em485->has_temt_isr) {
>> +			if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>> +				return;
>> +		} else {
>> +			while ((lsr & BOTH_EMPTY) != BOTH_EMPTY) {
>> +				lsr = serial_in(p, UART_LSR);
>> +				cpu_relax();
>> +			}
> 
> What happens if TEMP never be empty after interruption occurring?
> 

I've added the field has_temt_isr to identify if peripheral provides or 
not TEMT interrupt. In DW case, differentely from OMAP case, there is 
not TEMT interrupt, at least on Datasheet I've found.
At this time I don't have access to latest Datasheet.

Specifying has_temt_isr = false during serial8250_em485_init(),
I assume TEMT interrupt is not available and also is not enabled.

IMHO the only possible case to loop inside there is if shift register is 
costantly filled, but soon or late it will get empty(hope I didn't miss 
something).

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.
  2018-06-04 18:50     ` Giulio Benetti
@ 2018-06-05 10:51       ` Matwey V. Kornilov
  2018-06-06  9:36         ` Giulio Benetti
                           ` (2 more replies)
  0 siblings, 3 replies; 50+ messages in thread
From: Matwey V. Kornilov @ 2018-06-05 10:51 UTC (permalink / raw)
  To: Giulio Benetti
  Cc: Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko, Kees Cook,
	Matthias Brugger, Allen Pais, Sean Young, Ed Blake,
	Stefan Potyra, Philipp Zabel, Joshua Scott, Vignesh R,
	Rolf Evers-Fischer, Aaron Sierra, Rafael Gago, Joel Stanley,
	Sean Wang, linux-serial, linux-kernel

2018-06-04 21:50 GMT+03:00 Giulio Benetti <giulio.benetti@micronovasrl.com>:
> Il 04/06/2018 19:40, Matwey V. Kornilov ha scritto:
>>
>> 01.06.2018 15:40, Giulio Benetti пишет:
>>>
>>> Some 8250 ports only have TEMT interrupt, so current implementation
>>> can't work for ports without it. The only chance to make it work is to
>>> loop-read on LSR register.
>>>
>>> With NO TEMT interrupt check if both TEMT and THRE are set looping on
>>> LSR register.
>>>
>>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>> ---
>>>   drivers/tty/serial/8250/8250.h      |  2 +-
>>>   drivers/tty/serial/8250/8250_dw.c   |  2 +-
>>>   drivers/tty/serial/8250/8250_omap.c |  2 +-
>>>   drivers/tty/serial/8250/8250_port.c | 26 ++++++++++++++++++--------
>>>   include/linux/serial_8250.h         |  1 +
>>>   5 files changed, 22 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250.h
>>> b/drivers/tty/serial/8250/8250.h
>>> index ebfb0bd5bef5..5c6ae5f69432 100644
>>> --- a/drivers/tty/serial/8250/8250.h
>>> +++ b/drivers/tty/serial/8250/8250.h
>>> @@ -136,7 +136,7 @@ void serial8250_rpm_put(struct uart_8250_port *p);
>>>   void serial8250_rpm_get_tx(struct uart_8250_port *p);
>>>   void serial8250_rpm_put_tx(struct uart_8250_port *p);
>>>   -int serial8250_em485_init(struct uart_8250_port *p);
>>> +int serial8250_em485_init(struct uart_8250_port *p, bool has_temt_isr);
>>>   void serial8250_em485_destroy(struct uart_8250_port *p);
>>>     static inline void serial8250_out_MCR(struct uart_8250_port *up, int
>>> value)
>>> diff --git a/drivers/tty/serial/8250/8250_dw.c
>>> b/drivers/tty/serial/8250/8250_dw.c
>>> index 0f8b4da03d4e..888280ff5451 100644
>>> --- a/drivers/tty/serial/8250/8250_dw.c
>>> +++ b/drivers/tty/serial/8250/8250_dw.c
>>> @@ -330,7 +330,7 @@ static int dw8250_rs485_config(struct uart_port *p,
>>>          * are idempotent
>>>          */
>>>         if (rs485->flags & SER_RS485_ENABLED) {
>>> -               int ret = serial8250_em485_init(up);
>>> +               int ret = serial8250_em485_init(up, false);
>>>                 if (ret) {
>>>                         rs485->flags &= ~SER_RS485_ENABLED;
>>> diff --git a/drivers/tty/serial/8250/8250_omap.c
>>> b/drivers/tty/serial/8250/8250_omap.c
>>> index 624b501fd253..ab483c8b30c8 100644
>>> --- a/drivers/tty/serial/8250/8250_omap.c
>>> +++ b/drivers/tty/serial/8250/8250_omap.c
>>> @@ -725,7 +725,7 @@ static int omap_8250_rs485_config(struct uart_port
>>> *port,
>>>          * are idempotent
>>>          */
>>>         if (rs485->flags & SER_RS485_ENABLED) {
>>> -               int ret = serial8250_em485_init(up);
>>> +               int ret = serial8250_em485_init(up, true);
>>>                 if (ret) {
>>>                         rs485->flags &= ~SER_RS485_ENABLED;
>>> diff --git a/drivers/tty/serial/8250/8250_port.c
>>> b/drivers/tty/serial/8250/8250_port.c
>>> index 95833cbc4338..e14badbbf181 100644
>>> --- a/drivers/tty/serial/8250/8250_port.c
>>> +++ b/drivers/tty/serial/8250/8250_port.c
>>> @@ -599,15 +599,16 @@ EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>>   /**
>>>    *    serial8250_em485_init() - put uart_8250_port into rs485 emulating
>>>    *    @p:     uart_8250_port port instance
>>> + *     @p:     bool specify if 8250 port has TEMT interrupt or not
>>>    *
>>>    *    The function is used to start rs485 software emulating on the
>>>    *    &struct uart_8250_port* @p. Namely, RTS is switched before/after
>>>    *    transmission. The function is idempotent, so it is safe to call
>>> it
>>>    *    multiple times.
>>>    *
>>> - *     The caller MUST enable interrupt on empty shift register before
>>> - *     calling serial8250_em485_init(). This interrupt is not a part of
>>> - *     8250 standard, but implementation defined.
>>> + *     If has_temt_isr is passed as true, the caller MUST enable
>>> interrupt
>>> + *     on empty shift register before calling serial8250_em485_init().
>>> + *     This interrupt is not a part of 8250 standard, but implementation
>>> defined.
>>>    *
>>>    *    The function is supposed to be called from .rs485_config callback
>>>    *    or from any other callback protected with p->port.lock spinlock.
>>> @@ -616,7 +617,7 @@ EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>>    *
>>>    *    Return 0 - success, -errno - otherwise
>>>    */
>>> -int serial8250_em485_init(struct uart_8250_port *p)
>>> +int serial8250_em485_init(struct uart_8250_port *p, bool has_temt_isr)
>>>   {
>>>         if (p->em485)
>>>                 return 0;
>>> @@ -633,6 +634,7 @@ int serial8250_em485_init(struct uart_8250_port *p)
>>>         p->em485->start_tx_timer.function =
>>> &serial8250_em485_handle_start_tx;
>>>         p->em485->port = p;
>>>         p->em485->active_timer = NULL;
>>> +       p->em485->has_temt_isr = has_temt_isr;
>>>         serial8250_em485_rts_after_send(p);
>>>         return 0;
>>> @@ -1517,11 +1519,19 @@ static inline void __stop_tx(struct
>>> uart_8250_port *p)
>>>                 /*
>>>                  * To provide required timeing 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.
>>> +                * shift register are empty. If 8250 port supports it,
>>> +                * it is for device driver to enable interrupt on TEMT.
>>> +                * Otherwise must loop-read until TEMT and THRE flags are
>>> set.
>>>                  */
>>> -               if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>>> -                       return;
>>> +               if (em485->has_temt_isr) {
>>> +                       if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>>> +                               return;
>>> +               } else {
>>> +                       while ((lsr & BOTH_EMPTY) != BOTH_EMPTY) {
>>> +                               lsr = serial_in(p, UART_LSR);
>>> +                               cpu_relax();
>>> +                       }
>>
>>
>> What happens if TEMP never be empty after interruption occurring?
>>
>
> I've added the field has_temt_isr to identify if peripheral provides or not
> TEMT interrupt. In DW case, differentely from OMAP case, there is not TEMT
> interrupt, at least on Datasheet I've found.
> At this time I don't have access to latest Datasheet.
>
> Specifying has_temt_isr = false during serial8250_em485_init(),
> I assume TEMT interrupt is not available and also is not enabled.
>
> IMHO the only possible case to loop inside there is if shift register is
> costantly filled, but soon or late it will get empty(hope I didn't miss
> something).

Well, I would not rely on this behavior. Eventually, powersave or
something else disable transmit with characters left in buffer, or
happens something like that.

Could I ask to split the series into fixes and new features? I see
that the fixes can be applied, probably it would be better to apply
them separately from discussing new features.

>
> --
> Giulio Benetti
> CTO
>
> MICRONOVA SRL
> Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
> Tel. 049/8931563 - Fax 049/8931346
> Cod.Fiscale - P.IVA 02663420285
> Capitale Sociale € 26.000 i.v.
> Iscritta al Reg. Imprese di Padova N. 02663420285
> Numero R.E.A. 258642



-- 
With best regards,
Matwey V. Kornilov

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.
  2018-06-05 10:51       ` Matwey V. Kornilov
@ 2018-06-06  9:36         ` Giulio Benetti
  2018-06-06  9:49         ` [PATCH 2/4] serial: 8250: Copy mctrl when register port Giulio Benetti
  2018-06-06  9:51         ` [PATCH 2/4] serial: 8250_dw: allow enable rs485 at boot time Giulio Benetti
  2 siblings, 0 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06  9:36 UTC (permalink / raw)
  To: Matwey V. Kornilov
  Cc: Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko, Kees Cook,
	Matthias Brugger, Allen Pais, Sean Young, Ed Blake,
	Stefan Potyra, Philipp Zabel, Joshua Scott, Vignesh R,
	Rolf Evers-Fischer, Aaron Sierra, Rafael Gago, Joel Stanley,
	Sean Wang, linux-serial, linux-kernel

Hi,

Il 05/06/2018 12:51, Matwey V. Kornilov ha scritto:
> Could I ask to split the series into fixes and new features? I see
> that the fixes can be applied, probably it would be better to apply
> them separately from discussing new features.

Sure, I'm going to send 2 different patchsets.

Thanks

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 2/4] serial: 8250: Copy mctrl when register port.
  2018-06-05 10:51       ` Matwey V. Kornilov
  2018-06-06  9:36         ` Giulio Benetti
@ 2018-06-06  9:49         ` Giulio Benetti
  2018-06-06  9:49           ` [PATCH 3/4] serial: 8250: Make em485_rts_after_send() set mctrl according to rts state Giulio Benetti
                             ` (3 more replies)
  2018-06-06  9:51         ` [PATCH 2/4] serial: 8250_dw: allow enable rs485 at boot time Giulio Benetti
  2 siblings, 4 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06  9:49 UTC (permalink / raw)
  Cc: matwey.kornilov, Giulio Benetti, Greg Kroah-Hartman, Jiri Slaby,
	Andy Shevchenko, Matthias Brugger, Kees Cook, Allen Pais,
	Sean Young, open list:SERIAL DRIVERS, open list

RS485 can modify mctrl on startup, especially when RTS_AFTER_SEND is on
TIOCM_RTS is set, then need to keep it set when registering port.

Copy mctrl to new port too.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/8250/8250_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index c8c2b260c681..c8e62fbd6570 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -993,6 +993,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 		uart->port.unthrottle	= up->port.unthrottle;
 		uart->port.rs485_config	= up->port.rs485_config;
 		uart->port.rs485	= up->port.rs485;
+		uart->port.mctrl	= up->port.mctrl;
 		uart->dma		= up->dma;
 		uart->em485		= up->em485;
 
-- 
2.17.1

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 3/4] serial: 8250: Make em485_rts_after_send() set mctrl according to rts state.
  2018-06-06  9:49         ` [PATCH 2/4] serial: 8250: Copy mctrl when register port Giulio Benetti
@ 2018-06-06  9:49           ` Giulio Benetti
  2018-06-06 12:02             ` Andy Shevchenko
  2018-06-06  9:49           ` [PATCH 4/4] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set Giulio Benetti
                             ` (2 subsequent siblings)
  3 siblings, 1 reply; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06  9:49 UTC (permalink / raw)
  Cc: matwey.kornilov, Giulio Benetti, Greg Kroah-Hartman, Jiri Slaby,
	Andy Shevchenko, Matthias Brugger, Kees Cook, Sean Young,
	Aaron Sierra, Tomas Melin, Sean Wang, Rafael Gago, Joel Stanley,
	open list:SERIAL DRIVERS, open list

When rs485 enabled and RTS_AFTER_SEND set on startup, need to preserve
mctrl status, because later functions will call set_mctrl passing
port->mctrl=0 overriding rts status, resulting in rts pin in
transmission when idle.

Make mctrl reflect rts pin state.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/8250/8250_port.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 95833cbc4338..c8c10b5ec6d6 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -562,10 +562,13 @@ static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
 {
 	unsigned char mcr = serial8250_in_MCR(p);
 
-	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
+	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND) {
 		mcr |= UART_MCR_RTS;
-	else
+		p->port.mctrl |= TIOCM_RTS;
+	} else {
 		mcr &= ~UART_MCR_RTS;
+		p->port.mctrl &= ~TIOCM_RTS;
+	}
 	serial8250_out_MCR(p, mcr);
 }
 
-- 
2.17.1

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 4/4] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set.
  2018-06-06  9:49         ` [PATCH 2/4] serial: 8250: Copy mctrl when register port Giulio Benetti
  2018-06-06  9:49           ` [PATCH 3/4] serial: 8250: Make em485_rts_after_send() set mctrl according to rts state Giulio Benetti
@ 2018-06-06  9:49           ` Giulio Benetti
  2018-06-06 12:03             ` Andy Shevchenko
  2018-06-06  9:49           ` [PATCH 1/4] serial: 8250: Copy em485 from port to real port Giulio Benetti
  2018-06-06 12:01           ` [PATCH 2/4] serial: 8250: Copy mctrl when register port Andy Shevchenko
  3 siblings, 1 reply; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06  9:49 UTC (permalink / raw)
  Cc: matwey.kornilov, Giulio Benetti, Greg Kroah-Hartman, Jiri Slaby,
	Andy Shevchenko, Matthias Brugger, Kees Cook, Sean Young,
	open list:SERIAL DRIVERS, open list

If rs485 is enabled and RTS_AFTER_SEND is set on startup need to keep
TIOCM_RTS asserted to keep rs485 transceiver in RX when idle.

Check if rs485 is on and RTS_AFTER_SEND is set and mask port->mctrl with
TIOCM_RTS too and not only TIOCM_DTR.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/serial_core.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 0466f9f08a91..06d9441f6d20 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2288,6 +2288,16 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 
 	if (port->type != PORT_UNKNOWN) {
 		unsigned long flags;
+		int rs485_on = port->rs485_config &&
+			(port->rs485.flags & SER_RS485_ENABLED);
+		int RTS_after_send = !!(port->rs485.flags &
+				SER_RS485_RTS_AFTER_SEND);
+		int mctrl;
+
+		if (rs485_on && RTS_after_send)
+			mctrl = port->mctrl & (TIOCM_DTR | TIOCM_RTS);
+		else
+			mctrl = port->mctrl & TIOCM_DTR;
 
 		uart_report_port(drv, port);
 
@@ -2300,7 +2310,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 		 * We probably don't need a spinlock around this, but
 		 */
 		spin_lock_irqsave(&port->lock, flags);
-		port->ops->set_mctrl(port, port->mctrl & TIOCM_DTR);
+		port->ops->set_mctrl(port, mctrl);
 		spin_unlock_irqrestore(&port->lock, flags);
 
 		/*
-- 
2.17.1

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 1/4] serial: 8250: Copy em485 from port to real port.
  2018-06-06  9:49         ` [PATCH 2/4] serial: 8250: Copy mctrl when register port Giulio Benetti
  2018-06-06  9:49           ` [PATCH 3/4] serial: 8250: Make em485_rts_after_send() set mctrl according to rts state Giulio Benetti
  2018-06-06  9:49           ` [PATCH 4/4] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set Giulio Benetti
@ 2018-06-06  9:49           ` Giulio Benetti
  2018-06-06 11:56             ` Andy Shevchenko
  2018-06-06 12:01           ` [PATCH 2/4] serial: 8250: Copy mctrl when register port Andy Shevchenko
  3 siblings, 1 reply; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06  9:49 UTC (permalink / raw)
  Cc: matwey.kornilov, Giulio Benetti, Greg Kroah-Hartman, Jiri Slaby,
	Andy Shevchenko, Matthias Brugger, Kees Cook, Allen Pais,
	Sean Young, open list:SERIAL DRIVERS, open list

em485 gets lost during serial8250_register_8250_port().

Copy em485 to final uart port.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/8250/8250_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 9342fc2ee7df..c8c2b260c681 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -994,6 +994,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 		uart->port.rs485_config	= up->port.rs485_config;
 		uart->port.rs485	= up->port.rs485;
 		uart->dma		= up->dma;
+		uart->em485		= up->em485;
 
 		/* Take tx_loadsz from fifosize if it wasn't set separately */
 		if (uart->port.fifosize && !uart->tx_loadsz)
-- 
2.17.1

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 2/4] serial: 8250_dw: allow enable rs485 at boot time
  2018-06-05 10:51       ` Matwey V. Kornilov
  2018-06-06  9:36         ` Giulio Benetti
  2018-06-06  9:49         ` [PATCH 2/4] serial: 8250: Copy mctrl when register port Giulio Benetti
@ 2018-06-06  9:51         ` Giulio Benetti
  2018-06-06  9:51           ` [PATCH 3/4] serial: 8250: Handle case port doesn't have TEMT interrupt using em485 Giulio Benetti
                             ` (2 more replies)
  2 siblings, 3 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06  9:51 UTC (permalink / raw)
  Cc: matwey.kornilov, Giulio Benetti, Andy Shevchenko,
	Greg Kroah-Hartman, Jiri Slaby, Joshua Scott, Stefan Potyra,
	Ed Blake, open list:SERIAL DRIVERS, open list

If "linux,rs485-enabled-at-boot-time" is specified need to setup 485
in probe function.

Call uart_get_rs485_mode() to get rs485 configuration, then call
rs485_config() callback directly to setup port as rs485.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/8250/8250_dw.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 45366e6e5411..0f8b4da03d4e 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -582,8 +582,11 @@ static int dw8250_probe(struct platform_device *pdev)
 	if (data->uart_16550_compatible)
 		p->handle_irq = NULL;
 
-	if (!data->skip_autocfg)
+	if (!data->skip_autocfg) {
 		dw8250_setup_port(p);
+		uart_get_rs485_mode(dev, &p->rs485);
+		dw8250_rs485_config(p, &p->rs485);
+	}
 
 	/* If we have a valid fifosize, try hooking up DMA */
 	if (p->fifosize) {
-- 
2.17.1

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 3/4] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.
  2018-06-06  9:51         ` [PATCH 2/4] serial: 8250_dw: allow enable rs485 at boot time Giulio Benetti
@ 2018-06-06  9:51           ` Giulio Benetti
  2018-06-13 16:59             ` Alan Cox
  2018-06-06  9:51           ` [PATCH 4/4] serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and RTS_AFTER_SEND Giulio Benetti
  2018-06-06  9:51           ` [PATCH 1/4] serial: 8250_dw: add em485 support Giulio Benetti
  2 siblings, 1 reply; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06  9:51 UTC (permalink / raw)
  Cc: matwey.kornilov, Giulio Benetti, Greg Kroah-Hartman, Jiri Slaby,
	Andy Shevchenko, Stefan Potyra, Philipp Zabel, Ed Blake,
	Joshua Scott, Vignesh R, Rolf Evers-Fischer, Aaron Sierra,
	Phil Elwell, Rafael Gago, Joel Stanley, Sean Wang,
	open list:SERIAL DRIVERS, open list

Some 8250 ports only have TEMT interrupt, so current implementation
can't work for ports without it. The only chance to make it work is to
loop-read on LSR register.

With NO TEMT interrupt check if both TEMT and THRE are set looping on
LSR register.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/8250/8250.h      |  2 +-
 drivers/tty/serial/8250/8250_dw.c   |  2 +-
 drivers/tty/serial/8250/8250_omap.c |  2 +-
 drivers/tty/serial/8250/8250_port.c | 26 ++++++++++++++++++--------
 include/linux/serial_8250.h         |  1 +
 5 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ebfb0bd5bef5..5c6ae5f69432 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -136,7 +136,7 @@ void serial8250_rpm_put(struct uart_8250_port *p);
 void serial8250_rpm_get_tx(struct uart_8250_port *p);
 void serial8250_rpm_put_tx(struct uart_8250_port *p);
 
-int serial8250_em485_init(struct uart_8250_port *p);
+int serial8250_em485_init(struct uart_8250_port *p, bool has_temt_isr);
 void serial8250_em485_destroy(struct uart_8250_port *p);
 
 static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 0f8b4da03d4e..888280ff5451 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -330,7 +330,7 @@ static int dw8250_rs485_config(struct uart_port *p,
 	 * are idempotent
 	 */
 	if (rs485->flags & SER_RS485_ENABLED) {
-		int ret = serial8250_em485_init(up);
+		int ret = serial8250_em485_init(up, false);
 
 		if (ret) {
 			rs485->flags &= ~SER_RS485_ENABLED;
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 624b501fd253..ab483c8b30c8 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -725,7 +725,7 @@ static int omap_8250_rs485_config(struct uart_port *port,
 	 * are idempotent
 	 */
 	if (rs485->flags & SER_RS485_ENABLED) {
-		int ret = serial8250_em485_init(up);
+		int ret = serial8250_em485_init(up, true);
 
 		if (ret) {
 			rs485->flags &= ~SER_RS485_ENABLED;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index c8c10b5ec6d6..8432445c80e5 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -602,15 +602,16 @@ EXPORT_SYMBOL_GPL(serial8250_rpm_put);
 /**
  *	serial8250_em485_init() - put uart_8250_port into rs485 emulating
  *	@p:	uart_8250_port port instance
+ *	@p:	bool specify if 8250 port has TEMT interrupt or not
  *
  *	The function is used to start rs485 software emulating on the
  *	&struct uart_8250_port* @p. Namely, RTS is switched before/after
  *	transmission. The function is idempotent, so it is safe to call it
  *	multiple times.
  *
- *	The caller MUST enable interrupt on empty shift register before
- *	calling serial8250_em485_init(). This interrupt is not a part of
- *	8250 standard, but implementation defined.
+ *	If has_temt_isr is passed as true, the caller MUST enable interrupt
+ *	on empty shift register before calling serial8250_em485_init().
+ *	This interrupt is not a part of	8250 standard, but implementation defined.
  *
  *	The function is supposed to be called from .rs485_config callback
  *	or from any other callback protected with p->port.lock spinlock.
@@ -619,7 +620,7 @@ EXPORT_SYMBOL_GPL(serial8250_rpm_put);
  *
  *	Return 0 - success, -errno - otherwise
  */
-int serial8250_em485_init(struct uart_8250_port *p)
+int serial8250_em485_init(struct uart_8250_port *p, bool has_temt_isr)
 {
 	if (p->em485)
 		return 0;
@@ -636,6 +637,7 @@ int serial8250_em485_init(struct uart_8250_port *p)
 	p->em485->start_tx_timer.function = &serial8250_em485_handle_start_tx;
 	p->em485->port = p;
 	p->em485->active_timer = NULL;
+	p->em485->has_temt_isr = has_temt_isr;
 	serial8250_em485_rts_after_send(p);
 
 	return 0;
@@ -1520,11 +1522,19 @@ static inline void __stop_tx(struct uart_8250_port *p)
 		/*
 		 * To provide required timeing 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.
+		 * shift register are empty. If 8250 port supports it,
+		 * it is for device driver to enable interrupt on TEMT.
+		 * Otherwise must loop-read until TEMT and THRE flags are set.
 		 */
-		if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
-			return;
+		if (em485->has_temt_isr) {
+			if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
+				return;
+		} else {
+			while ((lsr & BOTH_EMPTY) != BOTH_EMPTY) {
+				lsr = serial_in(p, UART_LSR);
+				cpu_relax();
+			}
+		}
 
 		em485->active_timer = NULL;
 
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index a27ef5f56431..9b13cf59726b 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -83,6 +83,7 @@ struct uart_8250_em485 {
 	struct hrtimer		start_tx_timer; /* "rs485 start tx" timer */
 	struct hrtimer		stop_tx_timer;  /* "rs485 stop tx" timer */
 	struct hrtimer		*active_timer;  /* pointer to active timer */
+	bool			has_temt_isr;	/* say if 8250 has TEMT interrupt or no */
 	struct uart_8250_port	*port;          /* for hrtimer callbacks */
 };
 
-- 
2.17.1

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 4/4] serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and RTS_AFTER_SEND
  2018-06-06  9:51         ` [PATCH 2/4] serial: 8250_dw: allow enable rs485 at boot time Giulio Benetti
  2018-06-06  9:51           ` [PATCH 3/4] serial: 8250: Handle case port doesn't have TEMT interrupt using em485 Giulio Benetti
@ 2018-06-06  9:51           ` Giulio Benetti
  2018-06-06  9:51           ` [PATCH 1/4] serial: 8250_dw: add em485 support Giulio Benetti
  2 siblings, 0 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06  9:51 UTC (permalink / raw)
  Cc: matwey.kornilov, Giulio Benetti, Andy Shevchenko,
	Greg Kroah-Hartman, Jiri Slaby, Stefan Potyra, Joshua Scott,
	Ed Blake, open list:SERIAL DRIVERS, open list

If rs485 is on and RTS_AFTER_SEND it means that in idle state rts pin
must be asserted, othwerwise rs485 transceiver will enter tx state.
dw8250 when clocks are stopped keeps rts line de-asserted(high),
resulting in keeping rs485 transceiver in tx state when port is idle.

Check if rs485 is on with RTS_AFTER_SEND set, if so return -EBUSY in
rpm_suspend,

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/8250/8250_dw.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 888280ff5451..6b0ee6dc8ad0 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -668,6 +668,11 @@ static int dw8250_resume(struct device *dev)
 static int dw8250_runtime_suspend(struct device *dev)
 {
 	struct dw8250_data *data = dev_get_drvdata(dev);
+	struct uart_8250_port *up = serial8250_get_port(data->line);
+	struct uart_port *p = &up->port;
+
+	if (p->rs485.flags & (SER_RS485_ENABLED | SER_RS485_RTS_AFTER_SEND))
+		return -EBUSY;
 
 	if (!IS_ERR(data->clk))
 		clk_disable_unprepare(data->clk);
-- 
2.17.1

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH 1/4] serial: 8250_dw: add em485 support
  2018-06-06  9:51         ` [PATCH 2/4] serial: 8250_dw: allow enable rs485 at boot time Giulio Benetti
  2018-06-06  9:51           ` [PATCH 3/4] serial: 8250: Handle case port doesn't have TEMT interrupt using em485 Giulio Benetti
  2018-06-06  9:51           ` [PATCH 4/4] serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and RTS_AFTER_SEND Giulio Benetti
@ 2018-06-06  9:51           ` Giulio Benetti
  2018-06-06 16:51             ` Andy Shevchenko
  2 siblings, 1 reply; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06  9:51 UTC (permalink / raw)
  Cc: matwey.kornilov, Giulio Benetti, Andy Shevchenko,
	Greg Kroah-Hartman, Jiri Slaby, Ed Blake, Joshua Scott,
	open list:SERIAL DRIVERS, open list

Need to use rs485 transceiver so let's use existing em485 485 emulation
layer on top of 8250.

Add rs485_config callback to port.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/tty/serial/8250/8250_dw.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 6fcdb90f616a..45366e6e5411 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -314,6 +314,36 @@ static void dw8250_set_ldisc(struct uart_port *p, struct ktermios *termios)
 	serial8250_do_set_ldisc(p, termios);
 }
 
+static int dw8250_rs485_config(struct uart_port *p,
+			       struct serial_rs485 *rs485)
+{
+	struct uart_8250_port *up = up_to_u8250p(p);
+
+	/* Clamp the delays to [0, 100ms] */
+	rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
+	rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send, 100U);
+
+	p->rs485 = *rs485;
+
+	/*
+	 * Both serial8250_em485_init and serial8250_em485_destroy
+	 * are idempotent
+	 */
+	if (rs485->flags & SER_RS485_ENABLED) {
+		int ret = serial8250_em485_init(up);
+
+		if (ret) {
+			rs485->flags &= ~SER_RS485_ENABLED;
+			p->rs485.flags &= ~SER_RS485_ENABLED;
+		}
+		return ret;
+	}
+
+	serial8250_em485_destroy(up);
+
+	return 0;
+}
+
 /*
  * dw8250_fallback_dma_filter will prevent the UART from getting just any free
  * channel on platforms that have DMA engines, but don't have any channels
@@ -449,6 +479,7 @@ static int dw8250_probe(struct platform_device *pdev)
 	p->serial_out	= dw8250_serial_out;
 	p->set_ldisc	= dw8250_set_ldisc;
 	p->set_termios	= dw8250_set_termios;
+	p->rs485_config = dw8250_rs485_config;
 
 	p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
 	if (!p->membase)
-- 
2.17.1

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.
  2018-06-06  9:49           ` [PATCH 1/4] serial: 8250: Copy em485 from port to real port Giulio Benetti
@ 2018-06-06 11:56             ` Andy Shevchenko
  2018-06-06 12:15               ` Giulio Benetti
  0 siblings, 1 reply; 50+ messages in thread
From: Andy Shevchenko @ 2018-06-06 11:56 UTC (permalink / raw)
  To: Giulio Benetti
  Cc: matwey.kornilov, Greg Kroah-Hartman, Jiri Slaby,
	Matthias Brugger, Kees Cook, Allen Pais, Sean Young,
	open list:SERIAL DRIVERS, open list

On Wed, 2018-06-06 at 11:49 +0200, Giulio Benetti wrote:
> em485 gets lost during serial8250_register_8250_port().
> 
> Copy em485 to final uart port.
> 

Is it needed at all?

The individual driver decides either to use software emulation (and
calls explicitly serial8250_em485_init() for that) or do HW assisted
stuff.

> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>  drivers/tty/serial/8250/8250_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c
> b/drivers/tty/serial/8250/8250_core.c
> index 9342fc2ee7df..c8c2b260c681 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -994,6 +994,7 @@ int serial8250_register_8250_port(struct
> uart_8250_port *up)
>  		uart->port.rs485_config	= up-
> >port.rs485_config;
>  		uart->port.rs485	= up->port.rs485;
>  		uart->dma		= up->dma;
> +		uart->em485		= up->em485;
>  
>  		/* Take tx_loadsz from fifosize if it wasn't set
> separately */
>  		if (uart->port.fifosize && !uart->tx_loadsz)

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 2/4] serial: 8250: Copy mctrl when register port.
  2018-06-06  9:49         ` [PATCH 2/4] serial: 8250: Copy mctrl when register port Giulio Benetti
                             ` (2 preceding siblings ...)
  2018-06-06  9:49           ` [PATCH 1/4] serial: 8250: Copy em485 from port to real port Giulio Benetti
@ 2018-06-06 12:01           ` Andy Shevchenko
  3 siblings, 0 replies; 50+ messages in thread
From: Andy Shevchenko @ 2018-06-06 12:01 UTC (permalink / raw)
  To: Giulio Benetti
  Cc: matwey.kornilov, Greg Kroah-Hartman, Jiri Slaby,
	Matthias Brugger, Kees Cook, Allen Pais, Sean Young,
	open list:SERIAL DRIVERS, open list

On Wed, 2018-06-06 at 11:49 +0200, Giulio Benetti wrote:
> RS485 can modify mctrl on startup, especially when RTS_AFTER_SEND is
> on
> TIOCM_RTS is set, then need to keep it set when registering port.
> 
> Copy mctrl to new port too.
> 

Not sure if it would be useful.
Seems the only em485 user, i.e. OMAP, does survive without that.

Moreover, often individual drivers override ->set_mctrl() callback.

So, real example is needed to explain why.

> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>  drivers/tty/serial/8250/8250_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c
> b/drivers/tty/serial/8250/8250_core.c
> index c8c2b260c681..c8e62fbd6570 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -993,6 +993,7 @@ int serial8250_register_8250_port(struct
> uart_8250_port *up)
>  		uart->port.unthrottle	= up->port.unthrottle;
>  		uart->port.rs485_config	= up-
> >port.rs485_config;
>  		uart->port.rs485	= up->port.rs485;
> +		uart->port.mctrl	= up->port.mctrl;
>  		uart->dma		= up->dma;
>  		uart->em485		= up->em485;
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 3/4] serial: 8250: Make em485_rts_after_send() set mctrl according to rts state.
  2018-06-06  9:49           ` [PATCH 3/4] serial: 8250: Make em485_rts_after_send() set mctrl according to rts state Giulio Benetti
@ 2018-06-06 12:02             ` Andy Shevchenko
  0 siblings, 0 replies; 50+ messages in thread
From: Andy Shevchenko @ 2018-06-06 12:02 UTC (permalink / raw)
  To: Giulio Benetti
  Cc: matwey.kornilov, Greg Kroah-Hartman, Jiri Slaby,
	Matthias Brugger, Kees Cook, Sean Young, Aaron Sierra,
	Tomas Melin, Sean Wang, Rafael Gago, Joel Stanley,
	open list:SERIAL DRIVERS, open list

On Wed, 2018-06-06 at 11:49 +0200, Giulio Benetti wrote:
> When rs485 enabled and RTS_AFTER_SEND set on startup, need to preserve
> mctrl status, because later functions will call set_mctrl passing
> port->mctrl=0 overriding rts status, resulting in rts pin in
> transmission when idle.
> 
> Make mctrl reflect rts pin state.
> 

This might make sense, I leave it to Matwey to Ack / NAK / etc.
But it also feels that patch 2/4 should be part of this change.

> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c
> b/drivers/tty/serial/8250/8250_port.c
> index 95833cbc4338..c8c10b5ec6d6 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -562,10 +562,13 @@ static inline void
> serial8250_em485_rts_after_send(struct uart_8250_port *p)
>  {
>  	unsigned char mcr = serial8250_in_MCR(p);
>  
> -	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
> +	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND) {
>  		mcr |= UART_MCR_RTS;
> -	else
> +		p->port.mctrl |= TIOCM_RTS;
> +	} else {
>  		mcr &= ~UART_MCR_RTS;
> +		p->port.mctrl &= ~TIOCM_RTS;
> +	}
>  	serial8250_out_MCR(p, mcr);
>  }
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/4] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set.
  2018-06-06  9:49           ` [PATCH 4/4] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set Giulio Benetti
@ 2018-06-06 12:03             ` Andy Shevchenko
  2018-06-06 12:07               ` Giulio Benetti
  0 siblings, 1 reply; 50+ messages in thread
From: Andy Shevchenko @ 2018-06-06 12:03 UTC (permalink / raw)
  To: Giulio Benetti
  Cc: matwey.kornilov, Greg Kroah-Hartman, Jiri Slaby,
	Matthias Brugger, Kees Cook, Sean Young,
	open list:SERIAL DRIVERS, open list

On Wed, 2018-06-06 at 11:49 +0200, Giulio Benetti wrote:
> If rs485 is enabled and RTS_AFTER_SEND is set on startup need to keep
> TIOCM_RTS asserted to keep rs485 transceiver in RX when idle.
> 
> Check if rs485 is on and RTS_AFTER_SEND is set and mask port->mctrl
> with
> TIOCM_RTS too and not only TIOCM_DTR.
> 

This one feels wrong to be in serial_core.c. Perhaps in 8250/8250*.c.

> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>  drivers/tty/serial/serial_core.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c
> b/drivers/tty/serial/serial_core.c
> index 0466f9f08a91..06d9441f6d20 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2288,6 +2288,16 @@ uart_configure_port(struct uart_driver *drv,
> struct uart_state *state,
>  
>  	if (port->type != PORT_UNKNOWN) {
>  		unsigned long flags;
> +		int rs485_on = port->rs485_config &&
> +			(port->rs485.flags & SER_RS485_ENABLED);
> +		int RTS_after_send = !!(port->rs485.flags &
> +				SER_RS485_RTS_AFTER_SEND);
> +		int mctrl;
> +
> +		if (rs485_on && RTS_after_send)
> +			mctrl = port->mctrl & (TIOCM_DTR |
> TIOCM_RTS);
> +		else
> +			mctrl = port->mctrl & TIOCM_DTR;
>  
>  		uart_report_port(drv, port);
>  
> @@ -2300,7 +2310,7 @@ uart_configure_port(struct uart_driver *drv,
> struct uart_state *state,
>  		 * We probably don't need a spinlock around this, but
>  		 */
>  		spin_lock_irqsave(&port->lock, flags);
> -		port->ops->set_mctrl(port, port->mctrl & TIOCM_DTR);
> +		port->ops->set_mctrl(port, mctrl);
>  		spin_unlock_irqrestore(&port->lock, flags);
>  
>  		/*

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 4/4] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set.
  2018-06-06 12:03             ` Andy Shevchenko
@ 2018-06-06 12:07               ` Giulio Benetti
  0 siblings, 0 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06 12:07 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: matwey.kornilov, Greg Kroah-Hartman, Jiri Slaby,
	Matthias Brugger, Kees Cook, Sean Young,
	open list:SERIAL DRIVERS, open list

Il 06/06/2018 14:03, Andy Shevchenko ha scritto:
> On Wed, 2018-06-06 at 11:49 +0200, Giulio Benetti wrote:
>> If rs485 is enabled and RTS_AFTER_SEND is set on startup need to keep
>> TIOCM_RTS asserted to keep rs485 transceiver in RX when idle.
>>
>> Check if rs485 is on and RTS_AFTER_SEND is set and mask port->mctrl
>> with
>> TIOCM_RTS too and not only TIOCM_DTR.
>>
> 
> This one feels wrong to be in serial_core.c. Perhaps in 8250/8250*.c.

I've tried to avoid modifying serial_core.c but if it masks mctrl only 
with TIOCM_DTR, it forces RTS unasserted.
Another way could be:
If rs485 ON and RTS_AFTER_SEND set, then ignore RTS driving in 
8250_set_mctrl, would it make sense?

Thanks

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.
  2018-06-06 11:56             ` Andy Shevchenko
@ 2018-06-06 12:15               ` Giulio Benetti
  2018-06-06 13:11                 ` Andy Shevchenko
  0 siblings, 1 reply; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06 12:15 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: matwey.kornilov, Greg Kroah-Hartman, Jiri Slaby,
	Matthias Brugger, Kees Cook, Allen Pais, Sean Young,
	open list:SERIAL DRIVERS, open list

Il 06/06/2018 13:56, Andy Shevchenko ha scritto:
> On Wed, 2018-06-06 at 11:49 +0200, Giulio Benetti wrote:
>> em485 gets lost during serial8250_register_8250_port().
>>
>> Copy em485 to final uart port.
>>
> 
> Is it needed at all?
> 
> The individual driver decides either to use software emulation (and
> calls explicitly serial8250_em485_init() for that) or do HW assisted
> stuff.

In 8250_dw.c, during probe(), I need to call dw8250_rs485_config() 
against local struct uart_8250_port uart = {};
Inside serial8250_register_8250_port() not all uart fields are 
copied(em485 too).
So after probe, em485 is NULL.

Another way could be to call dw8250_rs485_config() against real uart 
port, after calling serial8250_register_8250_port(),
would it make sense?

> 
>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>> ---
>>   drivers/tty/serial/8250/8250_core.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/tty/serial/8250/8250_core.c
>> b/drivers/tty/serial/8250/8250_core.c
>> index 9342fc2ee7df..c8c2b260c681 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -994,6 +994,7 @@ int serial8250_register_8250_port(struct
>> uart_8250_port *up)
>>   		uart->port.rs485_config	= up-
>>> port.rs485_config;
>>   		uart->port.rs485	= up->port.rs485;
>>   		uart->dma		= up->dma;
>> +		uart->em485		= up->em485;
>>   
>>   		/* Take tx_loadsz from fifosize if it wasn't set
>> separately */
>>   		if (uart->port.fifosize && !uart->tx_loadsz)
> 

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.
  2018-06-06 12:15               ` Giulio Benetti
@ 2018-06-06 13:11                 ` Andy Shevchenko
  2018-06-06 14:32                   ` Giulio Benetti
  2018-06-06 18:55                   ` Matwey V. Kornilov
  0 siblings, 2 replies; 50+ messages in thread
From: Andy Shevchenko @ 2018-06-06 13:11 UTC (permalink / raw)
  To: Giulio Benetti
  Cc: matwey.kornilov, Greg Kroah-Hartman, Jiri Slaby,
	Matthias Brugger, Kees Cook, Allen Pais, Sean Young,
	open list:SERIAL DRIVERS, open list

On Wed, 2018-06-06 at 14:15 +0200, Giulio Benetti wrote:
> Il 06/06/2018 13:56, Andy Shevchenko ha scritto:
> > On Wed, 2018-06-06 at 11:49 +0200, Giulio Benetti wrote:
> > > em485 gets lost during
> > > 
> > > Copy em485 to final uart port.
> > > 
> > 
> > Is it needed at all?
> > 
> > The individual driver decides either to use software emulation (and
> > calls explicitly serial8250_em485_init() for that) or do HW assisted
> > stuff.
> 
> In 8250_dw.c, during probe(), I need to call dw8250_rs485_config() 
> against local struct uart_8250_port uart = {};
> Inside serial8250_register_8250_port() not all uart fields are 
> copied(em485 too).
> So after probe, em485 is NULL.
> 
> Another way could be to call dw8250_rs485_config() against real uart 
> port, after calling serial8250_register_8250_port(),
> would it make sense?

Look at OMAP case closely. They have a callback to configure RS485 which
is called in uart_set_rs485_config()  which is called whenever user
space does TIOCGRS485 IOCTL.

So, it's completely driven by user space which makes sense by my
opinion.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 6/8] serial: 8250: Copy mctrl when register port.
  2018-06-01 12:40 ` [PATCH 6/8] serial: 8250: Copy mctrl when register port Giulio Benetti
@ 2018-06-06 14:31   ` Aaron Sierra
  2018-06-06 14:44     ` Giulio Benetti
  0 siblings, 1 reply; 50+ messages in thread
From: Aaron Sierra @ 2018-06-06 14:31 UTC (permalink / raw)
  To: Giulio Benetti
  Cc: Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko, Kees Cook,
	Matthias Brugger, Allen Pais, Sean Young, Ed Blake,
	Stefan Potyra, Philipp Zabel, Joshua Scott, Vignesh R,
	Rolf Evers-Fischer, Rafael Gago, Joel Stanley, Sean Wang,
	linux-serial, linux-kernel

----- Original Message -----
> From: "Giulio Benetti" <giulio.benetti@micronovasrl.com>
> Sent: Friday, June 1, 2018 7:40:19 AM

> RS485 can modify mctrl on startup, especially when RTS_AFTER_SEND is on
> TIOCM_RTS is set, then need to keep it set when registering port.
> 
> Copy mctrl to new port too.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
> drivers/tty/serial/8250/8250_core.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c
> b/drivers/tty/serial/8250/8250_core.c
> index c8c2b260c681..c8e62fbd6570 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -993,6 +993,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
> 		uart->port.unthrottle	= up->port.unthrottle;
> 		uart->port.rs485_config	= up->port.rs485_config;
> 		uart->port.rs485	= up->port.rs485;
> +		uart->port.mctrl	= up->port.mctrl;

Hi Guilio,

I ran into this same thing about six months ago, but I was able to
accomplish what I needed by assigning a set_mctrl() function in my
port definition. Perhaps that would be enough for your case, too?

You should see a little lower in this file that set_mctrl is copied
to the new port.

-Aaron

> 		uart->dma		= up->dma;
> 		uart->em485		= up->em485;
> 
> --
> 2.17.0

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.
  2018-06-06 13:11                 ` Andy Shevchenko
@ 2018-06-06 14:32                   ` Giulio Benetti
  2018-06-06 18:55                   ` Matwey V. Kornilov
  1 sibling, 0 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06 14:32 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: matwey.kornilov, Greg Kroah-Hartman, Jiri Slaby,
	Matthias Brugger, Kees Cook, Allen Pais, Sean Young,
	open list:SERIAL DRIVERS, open list

Hi Andy,

Il 06/06/2018 15:11, Andy Shevchenko ha scritto:
> On Wed, 2018-06-06 at 14:15 +0200, Giulio Benetti wrote:
>> Il 06/06/2018 13:56, Andy Shevchenko ha scritto:
>>> On Wed, 2018-06-06 at 11:49 +0200, Giulio Benetti wrote:
>>>> em485 gets lost during
>>>>
>>>> Copy em485 to final uart port.
>>>>
>>>
>>> Is it needed at all?
>>>
>>> The individual driver decides either to use software emulation (and
>>> calls explicitly serial8250_em485_init() for that) or do HW assisted
>>> stuff.
>>
>> In 8250_dw.c, during probe(), I need to call dw8250_rs485_config()
>> against local struct uart_8250_port uart = {};
>> Inside serial8250_register_8250_port() not all uart fields are
>> copied(em485 too).
>> So after probe, em485 is NULL.
>>
>> Another way could be to call dw8250_rs485_config() against real uart
>> port, after calling serial8250_register_8250_port(),
>> would it make sense?
> 
> Look at OMAP case closely. They have a callback to configure RS485 which
> is called in uart_set_rs485_config()  which is called whenever user
> space does TIOCGRS485 IOCTL.
> 
> So, it's completely driven by user space which makes sense by my
> opinion.

This is my problem, being driven only by userspace means that until you 
don't open ttyS* from there, dw8250_rs485_config() won't be called and 
rs485 won't be configured.
In the case you have RTS_AFTER_SEND and 
"linux,rs485-enabled-at-boot-time" in dts, you need to handle RTS correctly.
Without calling:
- uart_get_rs485_mode() to collect dts properties
and
- dw8250_rs485_config() to configure according properties
the result is RTS NOT asserted, then pin is HIGH, meaning that rs485 
transceiver will be in tx mode until port is open.

Other drivers I've watched to for insipiration are:
- atmel_serial.c
- fsl_lpuart.c
- imx.c
etc.

everything containing uart_get_rs485_mode().

The main difficulty to understand this without a scope is that on 
rs485.txt documentation the property "rs485-rts-active-low" is described as:
"drive RTS low when sending (default is high)"
Instead it should report:
"de-assert RTS when sending(pin high), default is asserted(pin low)"

Maybe I should send a patch for that also.
I ended to this conclusion because on every check of RTS_AFTER_SEND and 
SER_RS485_RTS_ON_SEND RTS is treated this way.

Try to take a look at uart_port_dtr_rts() in serial_core.c for example.
Or serial8250_em485_rts_after_send() in 8250_port.c

What do you think?
-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 6/8] serial: 8250: Copy mctrl when register port.
  2018-06-06 14:31   ` Aaron Sierra
@ 2018-06-06 14:44     ` Giulio Benetti
  0 siblings, 0 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06 14:44 UTC (permalink / raw)
  To: Aaron Sierra
  Cc: Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko, Kees Cook,
	Matthias Brugger, Allen Pais, Sean Young, Ed Blake,
	Stefan Potyra, Philipp Zabel, Joshua Scott, Vignesh R,
	Rolf Evers-Fischer, Rafael Gago, Joel Stanley, Sean Wang,
	linux-serial, linux-kernel

Hi Aaron,

Il 06/06/2018 16:31, Aaron Sierra ha scritto:
> ----- Original Message -----
>> From: "Giulio Benetti" <giulio.benetti@micronovasrl.com>
>> Sent: Friday, June 1, 2018 7:40:19 AM
> 
>> RS485 can modify mctrl on startup, especially when RTS_AFTER_SEND is on
>> TIOCM_RTS is set, then need to keep it set when registering port.
>>
>> Copy mctrl to new port too.
>>
>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>> ---
>> drivers/tty/serial/8250/8250_core.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/tty/serial/8250/8250_core.c
>> b/drivers/tty/serial/8250/8250_core.c
>> index c8c2b260c681..c8e62fbd6570 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -993,6 +993,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>> 		uart->port.unthrottle	= up->port.unthrottle;
>> 		uart->port.rs485_config	= up->port.rs485_config;
>> 		uart->port.rs485	= up->port.rs485;
>> +		uart->port.mctrl	= up->port.mctrl;
> 
> Hi Guilio,
> 
> I ran into this same thing about six months ago, but I was able to
> accomplish what I needed by assigning a set_mctrl() function in my
> port definition. Perhaps that would be enough for your case, too?

Thanks for pointing me.
But wouldn't it be better copying mctrl?
In any case, serial8250_register_8250_port() will call:
- uart_add_one_port()
- uart_configure_port()
- port->ops->set_mctrl(port, mctrl);

So it would be a double call IMHO.
Are there any drawbacks on doing what I'm saying?
Maybe I'm missing something.

> You should see a little lower in this file that set_mctrl is copied
> to the new port. > -Aaron
> 
>> 		uart->dma		= up->dma;
>> 		uart->em485		= up->em485;
>>
>> --
>> 2.17.0

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 1/4] serial: 8250_dw: add em485 support
  2018-06-06  9:51           ` [PATCH 1/4] serial: 8250_dw: add em485 support Giulio Benetti
@ 2018-06-06 16:51             ` Andy Shevchenko
  2018-06-06 19:16               ` Giulio Benetti
  0 siblings, 1 reply; 50+ messages in thread
From: Andy Shevchenko @ 2018-06-06 16:51 UTC (permalink / raw)
  To: Giulio Benetti
  Cc: Matwey V. Kornilov, Andy Shevchenko, Greg Kroah-Hartman,
	Jiri Slaby, Ed Blake, Joshua Scott, open list:SERIAL DRIVERS,
	open list

On Wed, Jun 6, 2018 at 12:51 PM, Giulio Benetti
<giulio.benetti@micronovasrl.com> wrote:
> Need to use rs485 transceiver so let's use existing em485 485 emulation
> layer on top of 8250.
>
> Add rs485_config callback to port.

Besides the fact the series lacks of cover letter, I think it should
be postponed until we get a clear understanding how RS485 is supposed
to be initialized and what exact problems you are trying to address.

>
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 6fcdb90f616a..45366e6e5411 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -314,6 +314,36 @@ static void dw8250_set_ldisc(struct uart_port *p, struct ktermios *termios)
>         serial8250_do_set_ldisc(p, termios);
>  }
>
> +static int dw8250_rs485_config(struct uart_port *p,
> +                              struct serial_rs485 *rs485)
> +{
> +       struct uart_8250_port *up = up_to_u8250p(p);
> +
> +       /* Clamp the delays to [0, 100ms] */
> +       rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
> +       rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send, 100U);
> +
> +       p->rs485 = *rs485;
> +
> +       /*
> +        * Both serial8250_em485_init and serial8250_em485_destroy
> +        * are idempotent
> +        */
> +       if (rs485->flags & SER_RS485_ENABLED) {
> +               int ret = serial8250_em485_init(up);
> +
> +               if (ret) {
> +                       rs485->flags &= ~SER_RS485_ENABLED;
> +                       p->rs485.flags &= ~SER_RS485_ENABLED;
> +               }
> +               return ret;
> +       }
> +
> +       serial8250_em485_destroy(up);
> +
> +       return 0;
> +}
> +
>  /*
>   * dw8250_fallback_dma_filter will prevent the UART from getting just any free
>   * channel on platforms that have DMA engines, but don't have any channels
> @@ -449,6 +479,7 @@ static int dw8250_probe(struct platform_device *pdev)
>         p->serial_out   = dw8250_serial_out;
>         p->set_ldisc    = dw8250_set_ldisc;
>         p->set_termios  = dw8250_set_termios;
> +       p->rs485_config = dw8250_rs485_config;
>
>         p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
>         if (!p->membase)
> --
> 2.17.1
>



-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.
  2018-06-06 13:11                 ` Andy Shevchenko
  2018-06-06 14:32                   ` Giulio Benetti
@ 2018-06-06 18:55                   ` Matwey V. Kornilov
  2018-06-06 19:15                     ` Giulio Benetti
  1 sibling, 1 reply; 50+ messages in thread
From: Matwey V. Kornilov @ 2018-06-06 18:55 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Giulio Benetti, Greg Kroah-Hartman, Jiri Slaby, Matthias Brugger,
	Kees Cook, Allen Pais, Sean Young, open list:SERIAL DRIVERS,
	open list

2018-06-06 16:11 GMT+03:00 Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> On Wed, 2018-06-06 at 14:15 +0200, Giulio Benetti wrote:
>> Il 06/06/2018 13:56, Andy Shevchenko ha scritto:
>> > On Wed, 2018-06-06 at 11:49 +0200, Giulio Benetti wrote:
>> > > em485 gets lost during
>> > >
>> > > Copy em485 to final uart port.
>> > >
>> >
>> > Is it needed at all?
>> >
>> > The individual driver decides either to use software emulation (and
>> > calls explicitly serial8250_em485_init() for that) or do HW assisted
>> > stuff.
>>
>> In 8250_dw.c, during probe(), I need to call dw8250_rs485_config()
>> against local struct uart_8250_port uart = {};
>> Inside serial8250_register_8250_port() not all uart fields are
>> copied(em485 too).
>> So after probe, em485 is NULL.
>>
>> Another way could be to call dw8250_rs485_config() against real uart
>> port, after calling serial8250_register_8250_port(),
>> would it make sense?
>
> Look at OMAP case closely. They have a callback to configure RS485 which
> is called in uart_set_rs485_config()  which is called whenever user
> space does TIOCGRS485 IOCTL.
>
> So, it's completely driven by user space which makes sense by my
> opinion.

AFAIU, Giulio wants to add support for rs485-enabled-at-boot-time
device tree option (see bindings/serial/rs485.txt for reference).
I suppose it is only important for use-case when rs485 used as slave
(peripheral role).

>
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy



-- 
With best regards,
Matwey V. Kornilov

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.
  2018-06-06 18:55                   ` Matwey V. Kornilov
@ 2018-06-06 19:15                     ` Giulio Benetti
  2018-06-07  7:03                       ` Matwey V. Kornilov
  0 siblings, 1 reply; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06 19:15 UTC (permalink / raw)
  To: Matwey V. Kornilov, Andy Shevchenko
  Cc: Greg Kroah-Hartman, Jiri Slaby, Matthias Brugger, Kees Cook,
	Allen Pais, Sean Young, open list:SERIAL DRIVERS, open list

Il 06/06/2018 20:55, Matwey V. Kornilov ha scritto:
> 2018-06-06 16:11 GMT+03:00 Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
>> On Wed, 2018-06-06 at 14:15 +0200, Giulio Benetti wrote:
>>> Il 06/06/2018 13:56, Andy Shevchenko ha scritto:
>>>> On Wed, 2018-06-06 at 11:49 +0200, Giulio Benetti wrote:
>>>>> em485 gets lost during
>>>>>
>>>>> Copy em485 to final uart port.
>>>>>
>>>>
>>>> Is it needed at all?
>>>>
>>>> The individual driver decides either to use software emulation (and
>>>> calls explicitly serial8250_em485_init() for that) or do HW assisted
>>>> stuff.
>>>
>>> In 8250_dw.c, during probe(), I need to call dw8250_rs485_config()
>>> against local struct uart_8250_port uart = {};
>>> Inside serial8250_register_8250_port() not all uart fields are
>>> copied(em485 too).
>>> So after probe, em485 is NULL.
>>>
>>> Another way could be to call dw8250_rs485_config() against real uart
>>> port, after calling serial8250_register_8250_port(),
>>> would it make sense?
>>
>> Look at OMAP case closely. They have a callback to configure RS485 which
>> is called in uart_set_rs485_config()  which is called whenever user
>> space does TIOCGRS485 IOCTL.
>>
>> So, it's completely driven by user space which makes sense by my
>> opinion.
> 
> AFAIU, Giulio wants to add support for rs485-enabled-at-boot-time
> device tree option (see bindings/serial/rs485.txt for reference).

Yes, I want to add support for "rs485-enabled-at-boot-time" property,
maybe I had to write better commit log and a cover letter. Sorry.

> I suppose it is only important for use-case when rs485 used as slave
> (peripheral role).

It's important also for master, because RTS, if RTS_AFTER_SEND is set, 
remains not asserted(trasnmission) until userspace opens that serial port.

Sorry again for not explaining myself well.

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 1/4] serial: 8250_dw: add em485 support
  2018-06-06 16:51             ` Andy Shevchenko
@ 2018-06-06 19:16               ` Giulio Benetti
  0 siblings, 0 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-06 19:16 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Matwey V. Kornilov, Andy Shevchenko, Greg Kroah-Hartman,
	Jiri Slaby, Ed Blake, Joshua Scott, open list:SERIAL DRIVERS,
	open list

Il 06/06/2018 18:51, Andy Shevchenko ha scritto:
> On Wed, Jun 6, 2018 at 12:51 PM, Giulio Benetti
> <giulio.benetti@micronovasrl.com> wrote:
>> Need to use rs485 transceiver so let's use existing em485 485 emulation
>> layer on top of 8250.
>>
>> Add rs485_config callback to port.
> 
> Besides the fact the series lacks of cover letter, I think it should

Sorry for that, next patchsets will have cover-letters.

> be postponed until we get a clear understanding how RS485 is supposed
> to be initialized and what exact problems you are trying to address.
> 
>>
>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>> ---
>>   drivers/tty/serial/8250/8250_dw.c | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
>> index 6fcdb90f616a..45366e6e5411 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
>> @@ -314,6 +314,36 @@ static void dw8250_set_ldisc(struct uart_port *p, struct ktermios *termios)
>>          serial8250_do_set_ldisc(p, termios);
>>   }
>>
>> +static int dw8250_rs485_config(struct uart_port *p,
>> +                              struct serial_rs485 *rs485)
>> +{
>> +       struct uart_8250_port *up = up_to_u8250p(p);
>> +
>> +       /* Clamp the delays to [0, 100ms] */
>> +       rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
>> +       rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send, 100U);
>> +
>> +       p->rs485 = *rs485;
>> +
>> +       /*
>> +        * Both serial8250_em485_init and serial8250_em485_destroy
>> +        * are idempotent
>> +        */
>> +       if (rs485->flags & SER_RS485_ENABLED) {
>> +               int ret = serial8250_em485_init(up);
>> +
>> +               if (ret) {
>> +                       rs485->flags &= ~SER_RS485_ENABLED;
>> +                       p->rs485.flags &= ~SER_RS485_ENABLED;
>> +               }
>> +               return ret;
>> +       }
>> +
>> +       serial8250_em485_destroy(up);
>> +
>> +       return 0;
>> +}
>> +
>>   /*
>>    * dw8250_fallback_dma_filter will prevent the UART from getting just any free
>>    * channel on platforms that have DMA engines, but don't have any channels
>> @@ -449,6 +479,7 @@ static int dw8250_probe(struct platform_device *pdev)
>>          p->serial_out   = dw8250_serial_out;
>>          p->set_ldisc    = dw8250_set_ldisc;
>>          p->set_termios  = dw8250_set_termios;
>> +       p->rs485_config = dw8250_rs485_config;
>>
>>          p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
>>          if (!p->membase)
>> --
>> 2.17.1
>>
> 
> 
> 

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.
  2018-06-06 19:15                     ` Giulio Benetti
@ 2018-06-07  7:03                       ` Matwey V. Kornilov
  2018-06-07 12:43                         ` Giulio Benetti
  0 siblings, 1 reply; 50+ messages in thread
From: Matwey V. Kornilov @ 2018-06-07  7:03 UTC (permalink / raw)
  To: Giulio Benetti
  Cc: Andy Shevchenko, Greg Kroah-Hartman, Jiri Slaby,
	Matthias Brugger, Kees Cook, Allen Pais, Sean Young,
	open list:SERIAL DRIVERS, open list

2018-06-06 22:15 GMT+03:00 Giulio Benetti <giulio.benetti@micronovasrl.com>:
> Il 06/06/2018 20:55, Matwey V. Kornilov ha scritto:
>>
>> 2018-06-06 16:11 GMT+03:00 Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com>:
>>>
>>> On Wed, 2018-06-06 at 14:15 +0200, Giulio Benetti wrote:
>>>>
>>>> Il 06/06/2018 13:56, Andy Shevchenko ha scritto:
>>>>>
>>>>> On Wed, 2018-06-06 at 11:49 +0200, Giulio Benetti wrote:
>>>>>>
>>>>>> em485 gets lost during
>>>>>>
>>>>>> Copy em485 to final uart port.
>>>>>>
>>>>>
>>>>> Is it needed at all?
>>>>>
>>>>> The individual driver decides either to use software emulation (and
>>>>> calls explicitly serial8250_em485_init() for that) or do HW assisted
>>>>> stuff.
>>>>
>>>>
>>>> In 8250_dw.c, during probe(), I need to call dw8250_rs485_config()
>>>> against local struct uart_8250_port uart = {};
>>>> Inside serial8250_register_8250_port() not all uart fields are
>>>> copied(em485 too).
>>>> So after probe, em485 is NULL.
>>>>
>>>> Another way could be to call dw8250_rs485_config() against real uart
>>>> port, after calling serial8250_register_8250_port(),
>>>> would it make sense?
>>>
>>>
>>> Look at OMAP case closely. They have a callback to configure RS485 which
>>> is called in uart_set_rs485_config()  which is called whenever user
>>> space does TIOCGRS485 IOCTL.
>>>
>>> So, it's completely driven by user space which makes sense by my
>>> opinion.
>>
>>
>> AFAIU, Giulio wants to add support for rs485-enabled-at-boot-time
>> device tree option (see bindings/serial/rs485.txt for reference).
>
>
> Yes, I want to add support for "rs485-enabled-at-boot-time" property,
> maybe I had to write better commit log and a cover letter. Sorry.
>
>> I suppose it is only important for use-case when rs485 used as slave
>> (peripheral role).
>
>
> It's important also for master, because RTS, if RTS_AFTER_SEND is set,
> remains not asserted(trasnmission) until userspace opens that serial port.
>

And it makes no sense, because how are you going to transmit and
receive not opening the port and not running an application?
It is master who transmits the data first. So, before all systems
going to work you have to open the port from userspace.

In case of slave this of course makes sense.

> Sorry again for not explaining myself well.
>
>
> --
> Giulio Benetti
> CTO
>
> MICRONOVA SRL
> Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
> Tel. 049/8931563 - Fax 049/8931346
> Cod.Fiscale - P.IVA 02663420285
> Capitale Sociale € 26.000 i.v.
> Iscritta al Reg. Imprese di Padova N. 02663420285
> Numero R.E.A. 258642



-- 
With best regards,
Matwey V. Kornilov

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.
  2018-06-07  7:03                       ` Matwey V. Kornilov
@ 2018-06-07 12:43                         ` Giulio Benetti
  0 siblings, 0 replies; 50+ messages in thread
From: Giulio Benetti @ 2018-06-07 12:43 UTC (permalink / raw)
  To: Matwey V. Kornilov
  Cc: Andy Shevchenko, Greg Kroah-Hartman, Jiri Slaby,
	Matthias Brugger, Kees Cook, Allen Pais, Sean Young,
	open list:SERIAL DRIVERS, open list

Il 07/06/2018 09:03, Matwey V. Kornilov ha scritto:
> 2018-06-06 22:15 GMT+03:00 Giulio Benetti <giulio.benetti@micronovasrl.com>:
>> Il 06/06/2018 20:55, Matwey V. Kornilov ha scritto:
>>>
>>> 2018-06-06 16:11 GMT+03:00 Andy Shevchenko
>>> <andriy.shevchenko@linux.intel.com>:
>>>>
>>>> On Wed, 2018-06-06 at 14:15 +0200, Giulio Benetti wrote:
>>>>>
>>>>> Il 06/06/2018 13:56, Andy Shevchenko ha scritto:
>>>>>>
>>>>>> On Wed, 2018-06-06 at 11:49 +0200, Giulio Benetti wrote:
>>>>>>>
>>>>>>> em485 gets lost during
>>>>>>>
>>>>>>> Copy em485 to final uart port.
>>>>>>>
>>>>>>
>>>>>> Is it needed at all?
>>>>>>
>>>>>> The individual driver decides either to use software emulation (and
>>>>>> calls explicitly serial8250_em485_init() for that) or do HW assisted
>>>>>> stuff.
>>>>>
>>>>>
>>>>> In 8250_dw.c, during probe(), I need to call dw8250_rs485_config()
>>>>> against local struct uart_8250_port uart = {};
>>>>> Inside serial8250_register_8250_port() not all uart fields are
>>>>> copied(em485 too).
>>>>> So after probe, em485 is NULL.
>>>>>
>>>>> Another way could be to call dw8250_rs485_config() against real uart
>>>>> port, after calling serial8250_register_8250_port(),
>>>>> would it make sense?
>>>>
>>>>
>>>> Look at OMAP case closely. They have a callback to configure RS485 which
>>>> is called in uart_set_rs485_config()  which is called whenever user
>>>> space does TIOCGRS485 IOCTL.
>>>>
>>>> So, it's completely driven by user space which makes sense by my
>>>> opinion.
>>>
>>>
>>> AFAIU, Giulio wants to add support for rs485-enabled-at-boot-time
>>> device tree option (see bindings/serial/rs485.txt for reference).
>>
>>
>> Yes, I want to add support for "rs485-enabled-at-boot-time" property,
>> maybe I had to write better commit log and a cover letter. Sorry.
>>
>>> I suppose it is only important for use-case when rs485 used as slave
>>> (peripheral role).
>>
>>
>> It's important also for master, because RTS, if RTS_AFTER_SEND is set,
>> remains not asserted(trasnmission) until userspace opens that serial port.
>>
> 
> And it makes no sense, because how are you going to transmit and
> receive not opening the port and not running an application?
> It is master who transmits the data first. So, before all systems
> going to work you have to open the port from userspace.
> 
> In case of slave this of course makes sense.
> 

Yes it's true, my mistake, it works as master, even if bus will be held 
busy by master until port is open.

As slave it keeps bus busy.

IMHO it would be the best to have rs485 free until port is open,
this also reflects the reality:
when rs485 ON and RTS_AFTER_SEND is set, if port is closed or open 
without transmitting RTS must be asserted(pin low, rx mode).
Agree?

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH 3/4] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.
  2018-06-06  9:51           ` [PATCH 3/4] serial: 8250: Handle case port doesn't have TEMT interrupt using em485 Giulio Benetti
@ 2018-06-13 16:59             ` Alan Cox
  0 siblings, 0 replies; 50+ messages in thread
From: Alan Cox @ 2018-06-13 16:59 UTC (permalink / raw)
  To: Giulio Benetti
  Cc: matwey.kornilov, Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko,
	Stefan Potyra, Philipp Zabel, Ed Blake, Joshua Scott, Vignesh R,
	Rolf Evers-Fischer, Aaron Sierra, Phil Elwell, Rafael Gago,
	Joel Stanley, Sean Wang, open list:SERIAL DRIVERS, open list

> +		} else {
> +			while ((lsr & BOTH_EMPTY) != BOTH_EMPTY) {
> +				lsr = serial_in(p, UART_LSR);
> +				cpu_relax();
> +			}
> +		}

This still needs a timeout in case some kind of hardware flow control line
is asserted and therefore the byte is staying put.

Alan

^ permalink raw reply	[flat|nested] 50+ messages in thread

end of thread, other threads:[~2018-06-13 17:01 UTC | newest]

Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-01 12:40 [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw Giulio Benetti
2018-06-01 12:40 ` [PATCH 1/8] serial: 8250_dw: add em485 support Giulio Benetti
2018-06-01 12:40 ` [PATCH 2/8] serial: 8250_dw: allow enable rs485 at boot time Giulio Benetti
2018-06-01 12:40 ` [PATCH 3/8] serial: 8250: Copy em485 from port to real port Giulio Benetti
2018-06-04 10:13   ` Andy Shevchenko
2018-06-04 10:52     ` Giulio Benetti
2018-06-01 12:40 ` [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485 Giulio Benetti
2018-06-04 10:17   ` Andy Shevchenko
2018-06-04 10:50     ` Giulio Benetti
2018-06-04 11:38       ` Andy Shevchenko
2018-06-04 11:50         ` Giulio Benetti
2018-06-04 12:26           ` Andy Shevchenko
2018-06-04 17:40   ` Matwey V. Kornilov
2018-06-04 18:50     ` Giulio Benetti
2018-06-05 10:51       ` Matwey V. Kornilov
2018-06-06  9:36         ` Giulio Benetti
2018-06-06  9:49         ` [PATCH 2/4] serial: 8250: Copy mctrl when register port Giulio Benetti
2018-06-06  9:49           ` [PATCH 3/4] serial: 8250: Make em485_rts_after_send() set mctrl according to rts state Giulio Benetti
2018-06-06 12:02             ` Andy Shevchenko
2018-06-06  9:49           ` [PATCH 4/4] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set Giulio Benetti
2018-06-06 12:03             ` Andy Shevchenko
2018-06-06 12:07               ` Giulio Benetti
2018-06-06  9:49           ` [PATCH 1/4] serial: 8250: Copy em485 from port to real port Giulio Benetti
2018-06-06 11:56             ` Andy Shevchenko
2018-06-06 12:15               ` Giulio Benetti
2018-06-06 13:11                 ` Andy Shevchenko
2018-06-06 14:32                   ` Giulio Benetti
2018-06-06 18:55                   ` Matwey V. Kornilov
2018-06-06 19:15                     ` Giulio Benetti
2018-06-07  7:03                       ` Matwey V. Kornilov
2018-06-07 12:43                         ` Giulio Benetti
2018-06-06 12:01           ` [PATCH 2/4] serial: 8250: Copy mctrl when register port Andy Shevchenko
2018-06-06  9:51         ` [PATCH 2/4] serial: 8250_dw: allow enable rs485 at boot time Giulio Benetti
2018-06-06  9:51           ` [PATCH 3/4] serial: 8250: Handle case port doesn't have TEMT interrupt using em485 Giulio Benetti
2018-06-13 16:59             ` Alan Cox
2018-06-06  9:51           ` [PATCH 4/4] serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and RTS_AFTER_SEND Giulio Benetti
2018-06-06  9:51           ` [PATCH 1/4] serial: 8250_dw: add em485 support Giulio Benetti
2018-06-06 16:51             ` Andy Shevchenko
2018-06-06 19:16               ` Giulio Benetti
2018-06-01 12:40 ` [PATCH 5/8] serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and RTS_AFTER_SEND Giulio Benetti
2018-06-01 12:40 ` [PATCH 6/8] serial: 8250: Copy mctrl when register port Giulio Benetti
2018-06-06 14:31   ` Aaron Sierra
2018-06-06 14:44     ` Giulio Benetti
2018-06-01 12:40 ` [PATCH 7/8] serial: 8250: Make em485_rts_after_send() set mctrl according to rts state Giulio Benetti
2018-06-01 12:40 ` [PATCH 8/8] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set Giulio Benetti
2018-06-04 10:12 ` [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw Andy Shevchenko
2018-06-04 10:34   ` Matwey V. Kornilov
2018-06-04 10:42     ` Giulio Benetti
2018-06-04 11:44       ` Andy Shevchenko
2018-06-04 14:58         ` Giulio Benetti

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).