[PATCH 0/7] serial: 8250: Add rs485 emulation to 8250_dw

[PATCH 0/7] serial: 8250: Add rs485 emulation to 8250_dw

[Heiko Stuebner]

This series tries to revive the work of Giulio Benetti from 2018 [0]
which seemed to have stalled at that time.

The board I needed that on also had the additional caveat that it
uses non-standard pins for DE/RE so needed gpio mctrl layer as well
and even more special needed to control the RE pin manually not as
part of it being connected to the DE signal as seems to be the standard.

So I've marked the patch doing this as DTR pin as RFC but that patch
isn't needed for the other core functionality, so could also be left out.

Changes from the 2018 submission include:
- add timeout when waiting for fifos to clear using a new helper
- move on-boot enablement of the rs485 mode to after registering
  the port. This saves having to copy the em485 struct as done
  originally, which also ran into spinlock-debug warnings when testing
  and also makes it actually possible to use the mctrl gpio layer
  for non-standard gpios.

[0] Link: https://lore.kernel.org/linux-serial/20180601124021.102970-1-giulio.benetti@micronovasrl.com/

Giulio Benetti (4):
  serial: 8250: Make em485_rts_after_send() set mctrl according to rts
    state.
  serial: 8250: Handle case port doesn't have TEMT interrupt using
    em485.
  serial: 8250_dw: add em485 support
  serial: 8250_dw: allow enable rs485 at boot time

Heiko Stuebner (3):
  serial: 8250: add serial_in_poll_timeout helper
  serial: 8250: Start rs485 after registering port if rs485 is enabled
    in probe
  serial: 8250: handle DTR in rs485 emulation

 drivers/tty/serial/8250/8250.h      | 36 ++++++++++++++++++++-
 drivers/tty/serial/8250/8250_core.c |  9 ++++++
 drivers/tty/serial/8250/8250_dw.c   | 35 +++++++++++++++++++-
 drivers/tty/serial/8250/8250_of.c   |  2 +-
 drivers/tty/serial/8250/8250_omap.c |  2 +-
 drivers/tty/serial/8250/8250_port.c | 50 +++++++++++++++++++++++------
 include/linux/serial_8250.h         |  1 +
 7 files changed, 121 insertions(+), 14 deletions(-)

-- 
2.24.1

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

[Heiko Stuebner]

From: Giulio Benetti <giulio.benetti@micronovasrl.com>

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>
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.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 8407166610ce..67aa3a2a9afa 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -565,10 +565,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.24.1

[PATCH 2/7] serial: 8250: add serial_in_poll_timeout helper

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

In cases where a serial register needs to be polled until a specific
state, this should have a timeout as noted in the thread bringing em485
support to 8250_dw.

To not re-implement timeout handling in each case, add a helper modelled
after readx_poll_timeout / regmap_read_poll_timeout to facilitate this.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 drivers/tty/serial/8250/8250.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 33ad9d6de532..50a4c174410d 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -118,6 +118,40 @@ static inline void serial_out(struct uart_8250_port *up, int offset, int value)
 	up->port.serial_out(&up->port, offset, value);
 }
 
+/**
+ * serial_in_poll_timeout - Poll until a condition is met or a timeout occurs
+ *
+ * @port: uart_8250_port to read from
+ * @offs: Offset to poll
+ * @val: Integer variable to read the value into
+ * @cond: Break condition (usually involving @val)
+ * @timeout_us: Timeout in us, 0 means never timeout
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout or the serial_in
+ * error return value in case of a error read.
+ *
+ * This is modelled after the readx_poll_timeout macros in linux/iopoll.h.
+ */
+#define serial_in_poll_timeout(port, offs, val, cond, timeout_us) \
+({ \
+	u64 __timeout_us = (timeout_us); \
+	ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
+	for (;;) { \
+		val = serial_in((port), (offs)); \
+		if (val < 0) \
+			break; \
+		if (cond) \
+			break; \
+		if ((__timeout_us) && \
+		    ktime_compare(ktime_get(), __timeout) > 0) { \
+			val = serial_in((port), (offs)); \
+			break; \
+		} \
+		cpu_relax(); \
+	} \
+	(val < 0) ? val : ((cond) ? 0 : -ETIMEDOUT); \
+})
+
 void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p);
 
 static inline int serial_dl_read(struct uart_8250_port *up)
-- 
2.24.1

Re: [PATCH 2/7] serial: 8250: add serial_in_poll_timeout helper

[Andy Shevchenko]

On Wed, Mar 18, 2020 at 03:26:35PM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> In cases where a serial register needs to be polled until a specific
> state, this should have a timeout as noted in the thread bringing em485
> support to 8250_dw.
> 
> To not re-implement timeout handling in each case, add a helper modelled
> after readx_poll_timeout / regmap_read_poll_timeout to facilitate this.

> +#define serial_in_poll_timeout(port, offs, val, cond, timeout_us) \

This can (re-)use readx_poll_timeout().

Example:

https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?h=testing&id=b0415c224926c6d94c778d72f3d44c83862eb214

-- 
With Best Regards,
Andy Shevchenko

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

[Heiko Stuebner]

From: Giulio Benetti <giulio.benetti@micronovasrl.com>

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>
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 drivers/tty/serial/8250/8250.h      |  2 +-
 drivers/tty/serial/8250/8250_of.c   |  2 +-
 drivers/tty/serial/8250/8250_omap.c |  2 +-
 drivers/tty/serial/8250/8250_port.c | 30 +++++++++++++++++++++--------
 include/linux/serial_8250.h         |  1 +
 5 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 50a4c174410d..9e049d2a039e 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -190,7 +190,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);
 
 /* MCR <-> TIOCM conversion */
diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
index 92fbf46ce3bd..c77ab44a5468 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -64,7 +64,7 @@ static int of_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_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 836e736ae188..241322900298 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -734,7 +734,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 67aa3a2a9afa..3d1d8490bc42 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -605,15 +605,17 @@ 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.
@@ -622,7 +624,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;
@@ -639,6 +641,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;
@@ -1475,11 +1478,22 @@ 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 {
+			int val;
+
+			if (serial_in_poll_timeout(p, UART_LSR, val,
+					(val & BOTH_EMPTY) != BOTH_EMPTY,
+					100000) < 0)
+				pr_warn("%s: timeout waiting for fifos to empty\n",
+					p->port.name);
+		}
 
 		em485->active_timer = NULL;
 
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index bb2bc99388ca..c4b4469c272b 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -79,6 +79,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;	/* variant has TEMT interrupt */
 	struct uart_8250_port	*port;          /* for hrtimer callbacks */
 };
 
-- 
2.24.1

[PATCH 4/7] serial: 8250: Start rs485 after registering port if rs485 is enabled in probe

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

Before registering a port 8250 drivers may read devicetree information
regarding rs485 emulation, but starting the em485 emulation at that point
would only keep the em485 structure in the template uart port.

Also at that point before the port is registered possible gpios for
rts/dtr are not acquired yet as well.

So simply check after acquiring the port if the rs485-enable flag is set
and start the config callback in that case.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 drivers/tty/serial/8250/8250_core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index e682390ce0de..beab1c22b34d 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1068,6 +1068,15 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 		if (up->dl_write)
 			uart->dl_write = up->dl_write;
 
+		if (uart->port.rs485_config &&
+		    (uart->port.rs485.flags & SER_RS485_ENABLED)) {
+			dev_dbg(uart->port.dev, "starting in rs485 mode\n");
+			ret = uart->port.rs485_config(&uart->port,
+						      &uart->port.rs485);
+			if (ret < 0)
+				goto out_unlock;
+		}
+
 		if (uart->port.type != PORT_8250_CIR) {
 			if (serial8250_isa_config != NULL)
 				serial8250_isa_config(0, &uart->port,
-- 
2.24.1

[PATCH 5/7] serial: 8250: handle DTR in rs485 emulation

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

While in a lot of cases only RTS is needed as DE signal, sometimes the
RE signals is to be controlled separately, as DTR pin.

So add necessary control functions for it.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 drivers/tty/serial/8250/8250_port.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3d1d8490bc42..f8e5c096ed51 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -572,6 +572,10 @@ static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
 		mcr &= ~UART_MCR_RTS;
 		p->port.mctrl &= ~TIOCM_RTS;
 	}
+
+	mcr |= UART_MCR_DTR;
+	p->port.mctrl |= TIOCM_DTR;
+
 	serial8250_out_MCR(p, mcr);
 }
 
@@ -1558,6 +1562,13 @@ static inline void start_tx_rs485(struct uart_port *port)
 	em485->active_timer = NULL;
 
 	mcr = serial8250_in_MCR(up);
+
+	/* in rx_during_tx mode keep DTR active, to receive data */
+	if (up->port.rs485.flags & SER_RS485_RX_DURING_TX)
+		mcr |= UART_MCR_DTR;
+	else
+		mcr &= ~UART_MCR_DTR;
+
 	if (!!(up->port.rs485.flags & SER_RS485_RTS_ON_SEND) !=
 	    !!(mcr & UART_MCR_RTS)) {
 		if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
@@ -1572,6 +1583,8 @@ static inline void start_tx_rs485(struct uart_port *port)
 					 up->port.rs485.delay_rts_before_send);
 			return;
 		}
+	} else {
+		serial8250_out_MCR(up, mcr);
 	}
 
 	__start_tx(port);
-- 
2.24.1

[PATCH 6/7] serial: 8250_dw: add em485 support

[Heiko Stuebner]

From: Giulio Benetti <giulio.benetti@micronovasrl.com>

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>
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.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 1c72fdc2dd37..7023b656658d 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -320,6 +320,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, false);
+
+		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
@@ -417,6 +447,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.24.1

Re: [PATCH 6/7] serial: 8250_dw: add em485 support

[Andy Shevchenko]

On Wed, Mar 18, 2020 at 03:26:39PM +0100, Heiko Stuebner wrote:
> From: Giulio Benetti <giulio.benetti@micronovasrl.com>
> 
> Need to use rs485 transceiver so let's use existing em485 485 emulation
> layer on top of 8250.
> 
> Add rs485_config callback to port.

Synopsys DesignWare UART is being supported in three modules right now:
8250_dw (platform driver), 8250_dwlib (some library functions for this driver)
and 8250_lpss (PCI driver).

Can we do in a way that both platform and PCI drivers will get advantage by the change?

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

-- 
With Best Regards,
Andy Shevchenko

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

[Heiko Stuebner]

From: Giulio Benetti <giulio.benetti@micronovasrl.com>

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>
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 drivers/tty/serial/8250/8250_dw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 7023b656658d..c771faeaa260 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -550,8 +550,10 @@ 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);
+	}
 
 	/* If we have a valid fifosize, try hooking up DMA */
 	if (p->fifosize) {
-- 
2.24.1

Re: [PATCH 7/7] serial: 8250_dw: allow enable rs485 at boot time

[Andy Shevchenko]

On Wed, Mar 18, 2020 at 03:26:40PM +0100, Heiko Stuebner wrote:
> From: Giulio Benetti <giulio.benetti@micronovasrl.com>
> 
> 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.

I think you really need to Cc the new version of this to Lukas.
Because I have a deja vu that I have seen half of this to be similar what he
had done in his work.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/7] serial: 8250_dw: allow enable rs485 at boot time

[Heiko Stübner]

Am Mittwoch, 18. März 2020, 16:16:15 CET schrieb Andy Shevchenko:
> On Wed, Mar 18, 2020 at 03:26:40PM +0100, Heiko Stuebner wrote:
> > From: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > 
> > 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.
> 
> I think you really need to Cc the new version of this to Lukas.
> Because I have a deja vu that I have seen half of this to be similar what he
> had done in his work.

Could you give me a pointer to that work?

When I initially searched for previous rs485 on 8250_dw stuff I only
found the series from 2018 I linked to in the cover-letter, so if there
is other work somewhere else a pointer to it would be very helpful
for me.

Thanks
Heiko

Re: [PATCH 7/7] serial: 8250_dw: allow enable rs485 at boot time

[Heiko Stübner]

Am Mittwoch, 18. März 2020, 19:28:12 CET schrieb Heiko Stübner:
> Am Mittwoch, 18. März 2020, 16:16:15 CET schrieb Andy Shevchenko:
> > On Wed, Mar 18, 2020 at 03:26:40PM +0100, Heiko Stuebner wrote:
> > > From: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > > 
> > > 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.
> > 
> > I think you really need to Cc the new version of this to Lukas.
> > Because I have a deja vu that I have seen half of this to be similar what he
> > had done in his work.
> 
> Could you give me a pointer to that work?

Ignore this mail please, found the stuff in tty-next ;-)

Heiko


> When I initially searched for previous rs485 on 8250_dw stuff I only
> found the series from 2018 I linked to in the cover-letter, so if there
> is other work somewhere else a pointer to it would be very helpful
> for me.
> 
> Thanks
> Heiko
> 

Re: [PATCH 0/7] serial: 8250: Add rs485 emulation to 8250_dw

[Andy Shevchenko]

+Cc: Lukas, who did recently some work WRT RS485 support in 8250.

On Wed, Mar 18, 2020 at 03:26:33PM +0100, Heiko Stuebner wrote:
> This series tries to revive the work of Giulio Benetti from 2018 [0]
> which seemed to have stalled at that time.
> 
> The board I needed that on also had the additional caveat that it
> uses non-standard pins for DE/RE so needed gpio mctrl layer as well
> and even more special needed to control the RE pin manually not as
> part of it being connected to the DE signal as seems to be the standard.
> 
> So I've marked the patch doing this as DTR pin as RFC but that patch
> isn't needed for the other core functionality, so could also be left out.

Thank you, I'll look at them later on.

> Changes from the 2018 submission include:
> - add timeout when waiting for fifos to clear using a new helper
> - move on-boot enablement of the rs485 mode to after registering
>   the port. This saves having to copy the em485 struct as done
>   originally, which also ran into spinlock-debug warnings when testing
>   and also makes it actually possible to use the mctrl gpio layer
>   for non-standard gpios.
> 
> [0] Link: https://lore.kernel.org/linux-serial/20180601124021.102970-1-giulio.benetti@micronovasrl.com/
> 
> Giulio Benetti (4):
>   serial: 8250: Make em485_rts_after_send() set mctrl according to rts
>     state.
>   serial: 8250: Handle case port doesn't have TEMT interrupt using
>     em485.
>   serial: 8250_dw: add em485 support
>   serial: 8250_dw: allow enable rs485 at boot time
> 
> Heiko Stuebner (3):
>   serial: 8250: add serial_in_poll_timeout helper
>   serial: 8250: Start rs485 after registering port if rs485 is enabled
>     in probe
>   serial: 8250: handle DTR in rs485 emulation
> 
>  drivers/tty/serial/8250/8250.h      | 36 ++++++++++++++++++++-
>  drivers/tty/serial/8250/8250_core.c |  9 ++++++
>  drivers/tty/serial/8250/8250_dw.c   | 35 +++++++++++++++++++-
>  drivers/tty/serial/8250/8250_of.c   |  2 +-
>  drivers/tty/serial/8250/8250_omap.c |  2 +-
>  drivers/tty/serial/8250/8250_port.c | 50 +++++++++++++++++++++++------
>  include/linux/serial_8250.h         |  1 +
>  7 files changed, 121 insertions(+), 14 deletions(-)
> 
> -- 
> 2.24.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/7] serial: 8250: Add rs485 emulation to 8250_dw

[Lukas Wunner]

On Wed, Mar 18, 2020 at 04:43:20PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 18, 2020 at 03:26:33PM +0100, Heiko Stuebner wrote:
> > This series tries to revive the work of Giulio Benetti from 2018 [0]
> > which seemed to have stalled at that time.

Oh dear. :-(  This needs a rebase on current tty-next.

Patch [7/7] is already in tty-next as commit fe7f0fa43cef ("serial:
8250: Support rs485 devicetree properties").

Patch [4/7] likewise.  Note that it's not safe to call ->rs485_config()
already in serial8250_register_8250_port() if the driver uses UPF_IOREMAP
because ioremapping happens later via serial8250_config_port() ->
serial8250_request_std_resource(), so you'll get an oops for those
drivers when deasserting RTS early on.  Been there... :-(

Patch [6/7]:  Ugh, another duplication of the ->rs485_config() callback.
Just use the generic one introduced by commit 283e096ffb70 ("serial:
8250: Deduplicate ->rs485_config() callback") if possible.

The other patches appear to handle chip-specific needs.  It's now
possible to implement these in ->rs485_start_tx() and ->rs485_stop_tx()
hooks, as introduced by commit 058bc104f7ca ("serial: 8250: Generalize
rs485 software emulation").  Refer to commit f93bf7589114 ("serial:
8250_bcm2835aux: Support rs485 software emulation") for an example.

The DTR-for-RE thing seems a bit nonstandard, I'm not sure if this is
eligible for mainline or if it's something that should be kept in your
downstream tree.  But no harm in submitting it to the list.

Thanks,

Lukas

Re: [PATCH 0/7] serial: 8250: Add rs485 emulation to 8250_dw

[Andy Shevchenko]

On Wed, Mar 18, 2020 at 04:37:54PM +0100, Lukas Wunner wrote:
> On Wed, Mar 18, 2020 at 04:43:20PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 18, 2020 at 03:26:33PM +0100, Heiko Stuebner wrote:
> > > This series tries to revive the work of Giulio Benetti from 2018 [0]
> > > which seemed to have stalled at that time.
> 
> Oh dear. :-(  This needs a rebase on current tty-next.

That's what I was thinking when browsed thru the content, and thus commented in
the same way to this cover letter :-)

Thank you for looking into this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/7] serial: 8250: Add rs485 emulation to 8250_dw

[Heiko Stübner]

Hi,

Am Mittwoch, 18. März 2020, 16:37:54 CET schrieb Lukas Wunner:
> On Wed, Mar 18, 2020 at 04:43:20PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 18, 2020 at 03:26:33PM +0100, Heiko Stuebner wrote:
> > > This series tries to revive the work of Giulio Benetti from 2018 [0]
> > > which seemed to have stalled at that time.
> 
> Oh dear. :-(  This needs a rebase on current tty-next.

Looking at tty-next I notice that you're right. When I started working
on this I only found the stuff from 2018 I linked to but didn't imagine
that in that exact moment someone else would also work on that area.

So no worries, I'll adapt my code ;-)


> Patch [7/7] is already in tty-next as commit fe7f0fa43cef ("serial:
> 8250: Support rs485 devicetree properties").
>
> Patch [4/7] likewise.  Note that it's not safe to call ->rs485_config()
> already in serial8250_register_8250_port() if the driver uses UPF_IOREMAP
> because ioremapping happens later via serial8250_config_port() ->
> serial8250_request_std_resource(), so you'll get an oops for those
> drivers when deasserting RTS early on.  Been there... :-(
> 
> Patch [6/7]:  Ugh, another duplication of the ->rs485_config() callback.
> Just use the generic one introduced by commit 283e096ffb70 ("serial:
> 8250: Deduplicate ->rs485_config() callback") if possible.
> 
> The other patches appear to handle chip-specific needs.  It's now
> possible to implement these in ->rs485_start_tx() and ->rs485_stop_tx()
> hooks, as introduced by commit 058bc104f7ca ("serial: 8250: Generalize
> rs485 software emulation").  Refer to commit f93bf7589114 ("serial:
> 8250_bcm2835aux: Support rs485 software emulation") for an example.

Thanks for the pointers and also doing all that ground work :-)


> The DTR-for-RE thing seems a bit nonstandard, I'm not sure if this is
> eligible for mainline or if it's something that should be kept in your
> downstream tree.  But no harm in submitting it to the list.

I'm fine either way - maybe I also get a pointer on what may be a better
approach ;-)

At least DTR as "Data Terminal Ready" did sound somewhat matching for
the "Receive Enable" of RS485 (and it's also the only other output pin
in the mctrl gpio list).


Heiko

Re: [PATCH 0/7] serial: 8250: Add rs485 emulation to 8250_dw

[Lukas Wunner]

On Wed, Mar 18, 2020 at 07:49:08PM +0100, Heiko Stübner wrote:
> Looking at tty-next I notice that you're right. When I started working
> on this I only found the stuff from 2018 I linked to but didn't imagine
> that in that exact moment someone else would also work on that area.

There are some more patches in the pipeline for the next cycle
to add support for an rs485 bus termination GPIO.  They're on
the tip of this branch:

https://github.com/RevolutionPi/linux/commits/revpi-4.19

Just so you know in advance and duplicate work is avoided.


> > The DTR-for-RE thing seems a bit nonstandard, I'm not sure if this is
> > eligible for mainline or if it's something that should be kept in your
> > downstream tree.  But no harm in submitting it to the list.
> 
> I'm fine either way - maybe I also get a pointer on what may be a better
> approach ;-)
> 
> At least DTR as "Data Terminal Ready" did sound somewhat matching for
> the "Receive Enable" of RS485 (and it's also the only other output pin
> in the mctrl gpio list).

Some UARTs allow disabling the receiver, this can be taken advantage of
to implement half-duplex mode.  It's what I did in 8250_bcm2835aux.c.

On the Revolution Pi devices, !RE is usually connected to ground, so
reception is always enabled and it cannot be disabled in software
except by turning off the UART receiver.

There are also boards out there which connect !RE to RTS.  Then only
half-duplex mode is supported by the hardware and there's no way for
software to work around it.

Thanks,

Lukas

Re: [PATCH 0/7] serial: 8250: Add rs485 emulation to 8250_dw

[Heiko Stübner]

Hi,

Am Donnerstag, 19. März 2020, 06:40:34 CET schrieb Lukas Wunner:
> On Wed, Mar 18, 2020 at 07:49:08PM +0100, Heiko Stübner wrote:
> > Looking at tty-next I notice that you're right. When I started working
> > on this I only found the stuff from 2018 I linked to but didn't imagine
> > that in that exact moment someone else would also work on that area.
> 
> There are some more patches in the pipeline for the next cycle
> to add support for an rs485 bus termination GPIO.  They're on
> the tip of this branch:
> 
> https://github.com/RevolutionPi/linux/commits/revpi-4.19
> 
> Just so you know in advance and duplicate work is avoided.

do you plan on submitting these soonish? Because looking at your
termination-gpio change makes me want to do something similar for
my RE-gpio ... instead of trying to mangle this into the DTR thingy.


> > > The DTR-for-RE thing seems a bit nonstandard, I'm not sure if this is
> > > eligible for mainline or if it's something that should be kept in your
> > > downstream tree.  But no harm in submitting it to the list.
> > 
> > I'm fine either way - maybe I also get a pointer on what may be a better
> > approach ;-)
> > 
> > At least DTR as "Data Terminal Ready" did sound somewhat matching for
> > the "Receive Enable" of RS485 (and it's also the only other output pin
> > in the mctrl gpio list).
> 
> Some UARTs allow disabling the receiver, this can be taken advantage of
> to implement half-duplex mode.  It's what I did in 8250_bcm2835aux.c.
> 
> On the Revolution Pi devices, !RE is usually connected to ground, so
> reception is always enabled and it cannot be disabled in software
> except by turning off the UART receiver.
> 
> There are also boards out there which connect !RE to RTS.  Then only
> half-duplex mode is supported by the hardware and there's no way for
> software to work around it.

yep and on our board we have DE and RE on separate gpios, so I guess
it makes sense to use that pin how it was intended ;-) .

So I guess having that as rs485-re-gpios property might be the best way.


Heiko

Re: [PATCH 0/7] serial: 8250: Add rs485 emulation to 8250_dw

[Lukas Wunner]

On Mon, Mar 23, 2020 at 09:25:57AM +0100, Heiko Stübner wrote:
> Am Donnerstag, 19. März 2020, 06:40:34 CET schrieb Lukas Wunner:
> > There are some more patches in the pipeline for the next cycle
> > to add support for an rs485 bus termination GPIO.  They're on
> > the tip of this branch:
> > 
> > https://github.com/RevolutionPi/linux/commits/revpi-4.19
> > 
> > Just so you know in advance and duplicate work is avoided.
> 
> do you plan on submitting these soonish? Because looking at your
> termination-gpio change makes me want to do something similar for
> my RE-gpio ... instead of trying to mangle this into the DTR thingy.
[...]
> So I guess having that as rs485-re-gpios property might be the best way.

I plan to submit them once the 5.7 merge window closes, I'll probably
have to go over them at least one more time to apply some polish.

On UARTs capable of disabling and enabling the receiver in software,
it's best to leverage that to enable full-duplex or half-duplex mode.
However after having a brief look at the DW UART databook, it seems
it's not capable of doing that.  For such UARTs, a separate GPIO indeed
seems like a legitimate approach to allow switching between full-duplex
and half-duplex.

"rs485-re-gpios" seems a bit cryptic, how about "rs485-rx-enable-gpios"
or "rs485-full-duplex-gpios"?

Thanks,

Lukas

Re: [PATCH 0/7] serial: 8250: Add rs485 emulation to 8250_dw

[Andy Shevchenko]

On Mon, Mar 23, 2020 at 02:17:14PM +0100, Lukas Wunner wrote:
> On Mon, Mar 23, 2020 at 09:25:57AM +0100, Heiko Stübner wrote:
> > Am Donnerstag, 19. März 2020, 06:40:34 CET schrieb Lukas Wunner:

> "rs485-re-gpios" seems a bit cryptic, how about "rs485-rx-enable-gpios"
> or "rs485-full-duplex-gpios"?

First is in align with well established pin name, second is its elaboration,
I'm for any, but last.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/7] serial: 8250: Add rs485 emulation to 8250_dw

[Heiko Stübner]

Am Montag, 23. März 2020, 14:41:06 CET schrieb Andy Shevchenko:
> On Mon, Mar 23, 2020 at 02:17:14PM +0100, Lukas Wunner wrote:
> > On Mon, Mar 23, 2020 at 09:25:57AM +0100, Heiko Stübner wrote:
> > > Am Donnerstag, 19. März 2020, 06:40:34 CET schrieb Lukas Wunner:
> 
> > "rs485-re-gpios" seems a bit cryptic, how about "rs485-rx-enable-gpios"
> > or "rs485-full-duplex-gpios"?
> 
> First is in align with well established pin name, second is its elaboration,
> I'm for any, but last.

So I guess the intersection of both your preferences
is "rs485-rx-enable-gpios" ... and I did go with that ;-)

I've now moved my rs485 things over to tty-next + taking
	- "serial: Allow uart_get_rs485_mode() to return errno"
	- "serial: 8250: Support rs485 bus termination GPIO"
from Lukas' git-tree + fixing other things according to review comments.

I guess I'll now just sit on things till after the 5.7 merge window for
the depending patches to get posted.


Heiko

Re: [PATCH 0/7] serial: 8250: Add rs485 emulation to 8250_dw

[Andy Shevchenko]

On Wed, Mar 25, 2020 at 02:41:50PM +0100, Heiko Stübner wrote:
> Am Montag, 23. März 2020, 14:41:06 CET schrieb Andy Shevchenko:
> > On Mon, Mar 23, 2020 at 02:17:14PM +0100, Lukas Wunner wrote:
> > > On Mon, Mar 23, 2020 at 09:25:57AM +0100, Heiko Stübner wrote:
> > > > Am Donnerstag, 19. März 2020, 06:40:34 CET schrieb Lukas Wunner:
> > 
> > > "rs485-re-gpios" seems a bit cryptic, how about "rs485-rx-enable-gpios"
> > > or "rs485-full-duplex-gpios"?
> > 
> > First is in align with well established pin name, second is its elaboration,
> > I'm for any, but last.
> 
> So I guess the intersection of both your preferences
> is "rs485-rx-enable-gpios" ... and I did go with that ;-)


> I've now moved my rs485 things over to tty-next + taking
> 	- "serial: Allow uart_get_rs485_mode() to return errno"
> 	- "serial: 8250: Support rs485 bus termination GPIO"
> from Lukas' git-tree + fixing other things according to review comments.
> 
> I guess I'll now just sit on things till after the 5.7 merge window for
> the depending patches to get posted.

Sounds like a plan, thank you!

Or you can post anyway and resend after merge window. People will have a chance
to look at this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/7] serial: 8250: Add rs485 emulation to 8250_dw

[Andy Shevchenko]

On Wed, Mar 18, 2020 at 03:26:33PM +0100, Heiko Stuebner wrote:
> This series tries to revive the work of Giulio Benetti from 2018 [0]
> which seemed to have stalled at that time.
> 
> The board I needed that on also had the additional caveat that it
> uses non-standard pins for DE/RE so needed gpio mctrl layer as well
> and even more special needed to control the RE pin manually not as
> part of it being connected to the DE signal as seems to be the standard.
> 
> So I've marked the patch doing this as DTR pin as RFC but that patch
> isn't needed for the other core functionality, so could also be left out.

I'm wondering if this series is based on tty-next? Can you use --base in next version to clarify this?

> 
> Changes from the 2018 submission include:
> - add timeout when waiting for fifos to clear using a new helper
> - move on-boot enablement of the rs485 mode to after registering
>   the port. This saves having to copy the em485 struct as done
>   originally, which also ran into spinlock-debug warnings when testing
>   and also makes it actually possible to use the mctrl gpio layer
>   for non-standard gpios.
> 
> [0] Link: https://lore.kernel.org/linux-serial/20180601124021.102970-1-giulio.benetti@micronovasrl.com/
> 
> Giulio Benetti (4):
>   serial: 8250: Make em485_rts_after_send() set mctrl according to rts
>     state.
>   serial: 8250: Handle case port doesn't have TEMT interrupt using
>     em485.
>   serial: 8250_dw: add em485 support
>   serial: 8250_dw: allow enable rs485 at boot time
> 
> Heiko Stuebner (3):
>   serial: 8250: add serial_in_poll_timeout helper
>   serial: 8250: Start rs485 after registering port if rs485 is enabled
>     in probe
>   serial: 8250: handle DTR in rs485 emulation
> 
>  drivers/tty/serial/8250/8250.h      | 36 ++++++++++++++++++++-
>  drivers/tty/serial/8250/8250_core.c |  9 ++++++
>  drivers/tty/serial/8250/8250_dw.c   | 35 +++++++++++++++++++-
>  drivers/tty/serial/8250/8250_of.c   |  2 +-
>  drivers/tty/serial/8250/8250_omap.c |  2 +-
>  drivers/tty/serial/8250/8250_port.c | 50 +++++++++++++++++++++++------
>  include/linux/serial_8250.h         |  1 +
>  7 files changed, 121 insertions(+), 14 deletions(-)
> 
> -- 
> 2.24.1
> 

-- 
With Best Regards,
Andy Shevchenko