[PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw.

[PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw.

[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

[PATCH 1/8] serial: 8250_dw: add em485 support

[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

[PATCH 2/8] serial: 8250_dw: allow enable rs485 at boot time

[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

[PATCH 3/8] serial: 8250: Copy em485 from port to real port.

[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

[PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.

[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

[PATCH 5/8] serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and RTS_AFTER_SEND

[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

[PATCH 6/8] serial: 8250: Copy mctrl when register port.

[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

[PATCH 7/8] serial: 8250: Make em485_rts_after_send() set mctrl according to rts state.

[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

[PATCH 8/8] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set.

[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

Re: [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw.

[Andy Shevchenko]

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

Re: [PATCH 3/8] serial: 8250: Copy em485 from port to real port.

[Andy Shevchenko]

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

Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.

[Andy Shevchenko]

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

Re: [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw.

[Matwey V. Kornilov]

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

Re: [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw.

[Giulio Benetti]

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
>>
> 
> 
> 

Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.

[Giulio Benetti]

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

Re: [PATCH 3/8] serial: 8250: Copy em485 from port to real port.

[Giulio Benetti]

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

Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.

[Andy Shevchenko]

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

Re: [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw.

[Andy Shevchenko]

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

Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.

[Giulio Benetti]

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

Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.

[Andy Shevchenko]

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

Re: [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw.

[Giulio Benetti]

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

Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.

[Matwey V. Kornilov]

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 */
>  };
>  
> 

Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.

[Giulio Benetti]

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

Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.

[Matwey V. Kornilov]

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

Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.

[Giulio Benetti]

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

[PATCH 2/4] serial: 8250: Copy mctrl when register port.

[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.1

[PATCH 3/4] serial: 8250: Make em485_rts_after_send() set mctrl according to rts state.

[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 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

[PATCH 4/4] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set.

[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.1

[PATCH 1/4] serial: 8250: Copy em485 from port to real port.

[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.1

[PATCH 2/4] serial: 8250_dw: allow enable rs485 at boot time

[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.1

[PATCH 3/4] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.

[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 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

[PATCH 4/4] serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and RTS_AFTER_SEND

[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.1

[PATCH 1/4] serial: 8250_dw: add em485 support

[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.1

Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.

[Andy Shevchenko]

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

Re: [PATCH 2/4] serial: 8250: Copy mctrl when register port.

[Andy Shevchenko]

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

Re: [PATCH 3/4] serial: 8250: Make em485_rts_after_send() set mctrl according to rts state.

[Andy Shevchenko]

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

Re: [PATCH 4/4] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set.

[Andy Shevchenko]

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

Re: [PATCH 4/4] serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and RTS_AFTER_SEND set.

[Giulio Benetti]

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

Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.

[Giulio Benetti]

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

Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.

[Andy Shevchenko]

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

Re: [PATCH 6/8] serial: 8250: Copy mctrl when register port.

[Aaron Sierra]

----- 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

Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.

[Giulio Benetti]

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

Re: [PATCH 6/8] serial: 8250: Copy mctrl when register port.

[Giulio Benetti]

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

Re: [PATCH 1/4] serial: 8250_dw: add em485 support

[Andy Shevchenko]

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

Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.

[Matwey V. Kornilov]

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

Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.

[Giulio Benetti]

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

Re: [PATCH 1/4] serial: 8250_dw: add em485 support

[Giulio Benetti]

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

Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.

[Matwey V. Kornilov]

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

Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.

[Giulio Benetti]

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

Re: [PATCH 3/4] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.

[Alan Cox]

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