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

[PATCH RFC v2 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.

The first 3 patches are taken from Lukas Wunner's repository
with work he wants to submit after 5.7-rc1.
Patches needed adaptions from their 4.19 base of course,
so no guarantes I did that correctly ;-) .
I just need the change in the rs485_mode handling as base.

So as suggested by Andy this series is meant as a base for discussions
to show how my 8250_dw rs485 support fits onto the more recent codebase
and find more issues earlier.


Changes in v2:
- move to recent rs485 improvements in tty-next
- added a capability for TEMT interrupt presence
- external gpio for optional receiver-enable handling
- timeout polling via the generic readx_poll_timeout

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.


*** BLURB HERE ***

Giulio Benetti (2):
  serial: 8250: Handle implementations not having TEMT interrupt using
    em485
  serial: 8250_dw: add em485 support

Heiko Stuebner (2):
  dt-bindings: serial: Add binding for rs485 receiver enable GPIO
  serial: 8250: Support separate rs485 rx-enable GPIO

Lukas Wunner (3):
  serial: Allow uart_get_rs485_mode() to return errno
  dt-bindings: serial: Add binding for rs485 bus termination GPIO
  serial: 8250: Support rs485 bus termination GPIO

 .../devicetree/bindings/serial/rs485.yaml     |  8 ++++
 drivers/tty/serial/8250/8250.h                |  1 +
 drivers/tty/serial/8250/8250_bcm2835aux.c     |  2 +-
 drivers/tty/serial/8250/8250_core.c           |  4 +-
 drivers/tty/serial/8250/8250_dw.c             |  3 ++
 drivers/tty/serial/8250/8250_of.c             |  2 +
 drivers/tty/serial/8250/8250_omap.c           |  2 +-
 drivers/tty/serial/8250/8250_port.c           | 41 +++++++++++++++---
 drivers/tty/serial/ar933x_uart.c              |  6 ++-
 drivers/tty/serial/atmel_serial.c             |  6 ++-
 drivers/tty/serial/fsl_lpuart.c               |  5 ++-
 drivers/tty/serial/imx.c                      |  6 ++-
 drivers/tty/serial/omap-serial.c              |  4 +-
 drivers/tty/serial/serial_core.c              | 43 ++++++++++++++++++-
 drivers/tty/serial/stm32-usart.c              |  8 ++--
 include/linux/serial_core.h                   |  4 +-
 16 files changed, 124 insertions(+), 21 deletions(-)

-- 
2.24.1

[PATCH DON'T APPLY v2 1/7] serial: Allow uart_get_rs485_mode() to return errno

[Heiko Stuebner]

From: Lukas Wunner <lukas@wunner.de>

We're about to amend uart_get_rs485_mode() to support a GPIO pin for
rs485 bus termination.  Retrieving the GPIO descriptor may fail, so
allow uart_get_rs485_mode() to return an errno and change all callers
to check for failure.

The GPIO descriptor is going to be stored in struct uart_port.  Pass
that struct to uart_get_rs485_mode() in lieu of a struct device and
struct serial_rs485, both of which are directly accessible from struct
uart_port.

A few drivers call uart_get_rs485_mode() before setting the struct
device pointer in struct uart_port.  Shuffle those calls around where
necessary.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
[add ar933x_uart as well]
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/tty/serial/8250/8250_core.c | 4 +++-
 drivers/tty/serial/ar933x_uart.c    | 6 ++++--
 drivers/tty/serial/atmel_serial.c   | 6 ++++--
 drivers/tty/serial/fsl_lpuart.c     | 5 ++++-
 drivers/tty/serial/imx.c            | 6 +++++-
 drivers/tty/serial/omap-serial.c    | 4 +++-
 drivers/tty/serial/serial_core.c    | 6 +++++-
 drivers/tty/serial/stm32-usart.c    | 8 ++++----
 include/linux/serial_core.h         | 2 +-
 9 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 45d9117cab68..cbf370e22344 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1026,7 +1026,9 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 
 		if (up->port.dev) {
 			uart->port.dev = up->port.dev;
-			uart_get_rs485_mode(uart->port.dev, &uart->port.rs485);
+			ret = uart_get_rs485_mode(&uart->port);
+			if (ret)
+				goto out_unlock;
 		}
 
 		if (up->port.flags & UPF_FIXED_TYPE)
diff --git a/drivers/tty/serial/ar933x_uart.c b/drivers/tty/serial/ar933x_uart.c
index 7e7f1398019f..5bace041b94c 100644
--- a/drivers/tty/serial/ar933x_uart.c
+++ b/drivers/tty/serial/ar933x_uart.c
@@ -766,8 +766,6 @@ static int ar933x_uart_probe(struct platform_device *pdev)
 		goto err_disable_clk;
 	}
 
-	uart_get_rs485_mode(&pdev->dev, &port->rs485);
-
 	port->mapbase = mem_res->start;
 	port->line = id;
 	port->irq = irq_res->start;
@@ -780,6 +778,10 @@ static int ar933x_uart_probe(struct platform_device *pdev)
 	port->ops = &ar933x_uart_ops;
 	port->rs485_config = ar933x_config_rs485;
 
+	ret = uart_get_rs485_mode(port);
+	if (ret)
+		goto err_disable_clk;
+
 	baud = ar933x_uart_get_baud(port->uartclk, AR933X_UART_MAX_SCALE, 1);
 	up->min_baud = max_t(unsigned int, baud, AR933X_UART_MIN_BAUD);
 
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 8d7080efad9b..e43471b33710 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2491,8 +2491,6 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
 	atmel_init_property(atmel_port, pdev);
 	atmel_set_ops(port);
 
-	uart_get_rs485_mode(&mpdev->dev, &port->rs485);
-
 	port->iotype		= UPIO_MEM;
 	port->flags		= UPF_BOOT_AUTOCONF | UPF_IOREMAP;
 	port->ops		= &atmel_pops;
@@ -2506,6 +2504,10 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
 
 	memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
 
+	ret = uart_get_rs485_mode(port);
+	if (ret)
+		return ret;
+
 	/* for console, the clock could already be configured */
 	if (!atmel_port->clk) {
 		atmel_port->clk = clk_get(&mpdev->dev, "usart");
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 9c6a018b1390..4a1b507b4af1 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -2611,7 +2611,9 @@ static int lpuart_probe(struct platform_device *pdev)
 	if (ret)
 		goto failed_attach_port;
 
-	uart_get_rs485_mode(&pdev->dev, &sport->port.rs485);
+	ret = uart_get_rs485_mode(&sport->port);
+	if (ret)
+		goto failed_get_rs485;
 
 	if (sport->port.rs485.flags & SER_RS485_RX_DURING_TX)
 		dev_err(&pdev->dev, "driver doesn't support RX during TX\n");
@@ -2624,6 +2626,7 @@ static int lpuart_probe(struct platform_device *pdev)
 
 	return 0;
 
+failed_get_rs485:
 failed_attach_port:
 failed_irq_request:
 	lpuart_disable_clks(sport);
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index f4d68109bc8b..91f3910d6c44 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2302,7 +2302,11 @@ static int imx_uart_probe(struct platform_device *pdev)
 	sport->ucr4 = readl(sport->port.membase + UCR4);
 	sport->ufcr = readl(sport->port.membase + UFCR);
 
-	uart_get_rs485_mode(&pdev->dev, &sport->port.rs485);
+	ret = uart_get_rs485_mode(&sport->port);
+	if (ret) {
+		clk_disable_unprepare(sport->clk_ipg);
+		return ret;
+	}
 
 	if (sport->port.rs485.flags & SER_RS485_ENABLED &&
 	    (!sport->have_rtscts && !sport->have_rtsgpio))
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 48017cec7f2f..532fbc68e801 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1609,7 +1609,9 @@ static int serial_omap_probe_rs485(struct uart_omap_port *up,
 	if (!np)
 		return 0;
 
-	uart_get_rs485_mode(up->dev, rs485conf);
+	ret = uart_get_rs485_mode(&up->port);
+	if (ret)
+		return ret;
 
 	if (of_property_read_bool(np, "rs485-rts-active-high")) {
 		rs485conf->flags |= SER_RS485_RTS_ON_SEND;
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 66a5e2faf57e..43b6682877d5 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3295,8 +3295,10 @@ EXPORT_SYMBOL(uart_remove_one_port);
  * This function implements the device tree binding described in
  * Documentation/devicetree/bindings/serial/rs485.txt.
  */
-void uart_get_rs485_mode(struct device *dev, struct serial_rs485 *rs485conf)
+int uart_get_rs485_mode(struct uart_port *port)
 {
+	struct serial_rs485 *rs485conf = &port->rs485;
+	struct device *dev = port->dev;
 	u32 rs485_delay[2];
 	int ret;
 
@@ -3328,6 +3330,8 @@ void uart_get_rs485_mode(struct device *dev, struct serial_rs485 *rs485conf)
 		rs485conf->flags &= ~SER_RS485_RTS_ON_SEND;
 		rs485conf->flags |= SER_RS485_RTS_AFTER_SEND;
 	}
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(uart_get_rs485_mode);
 
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index 5e93e8d40f59..e3db54398159 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -158,9 +158,7 @@ static int stm32_init_rs485(struct uart_port *port,
 	if (!pdev->dev.of_node)
 		return -ENODEV;
 
-	uart_get_rs485_mode(&pdev->dev, rs485conf);
-
-	return 0;
+	return uart_get_rs485_mode(port);
 }
 
 static int stm32_pending_rx(struct uart_port *port, u32 *sr, int *last_res,
@@ -931,7 +929,9 @@ static int stm32_init_port(struct stm32_port *stm32port,
 
 	port->rs485_config = stm32_config_rs485;
 
-	stm32_init_rs485(port, pdev);
+	ret = stm32_init_rs485(port, pdev);
+	if (ret)
+		return ret;
 
 	if (stm32port->info->cfg.has_wakeup) {
 		stm32port->wakeirq = platform_get_irq(pdev, 1);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 92f5eba86052..b649a2b894e7 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -472,5 +472,5 @@ extern int uart_handle_break(struct uart_port *port);
 					 (cflag) & CRTSCTS || \
 					 !((cflag) & CLOCAL))
 
-void uart_get_rs485_mode(struct device *dev, struct serial_rs485 *rs485conf);
+int uart_get_rs485_mode(struct uart_port *port);
 #endif /* LINUX_SERIAL_CORE_H */
-- 
2.24.1

[PATCH DON'T APPLY v2 2/7] dt-bindings: serial: Add binding for rs485 bus termination GPIO

[Heiko Stuebner]

From: Lukas Wunner <lukas@wunner.de>

Commit e8759ad17d41 ("serial: uapi: Add support for bus termination")
introduced the ability to enable rs485 bus termination from user space.
So far the feature is only used by a single driver, 8250_exar.c, using a
hardcoded GPIO pin specific to Siemens IOT2040 products.

Provide for a more generic solution by allowing specification of an
rs485 bus termination GPIO pin in the device tree.  An upcoming commit
implements support for this pin for any 8250 driver.  The binding is
used in device trees of the "Revolution Pi" PLCs offered by KUNBUS.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Sascha Weisenberger <sascha.weisenberger@siemens.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
[moved to yaml rs485 binding]
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 Documentation/devicetree/bindings/serial/rs485.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/rs485.yaml b/Documentation/devicetree/bindings/serial/rs485.yaml
index d4beaf11222d..a9ad17864889 100644
--- a/Documentation/devicetree/bindings/serial/rs485.yaml
+++ b/Documentation/devicetree/bindings/serial/rs485.yaml
@@ -43,3 +43,7 @@ properties:
   rs485-rx-during-tx:
    description: enables the receiving of data even while sending data.
    $ref: /schemas/types.yaml#/definitions/flag
+
+  rs485-term-gpios:
+    description: GPIO pin to enable RS485 bus termination.
+    maxItems: 1
-- 
2.24.1

[PATCH DON'T APPLY v2 3/7] serial: 8250: Support rs485 bus termination GPIO

[Heiko Stuebner]

From: Lukas Wunner <lukas@wunner.de>

Amend the serial core to retrieve the rs485 bus termination GPIO from
the device tree (or ACPI table) and amend the default ->rs485_config()
callback for 8250 drivers to change the GPIO on request from user space.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/tty/serial/8250/8250_port.c |  5 +++++
 drivers/tty/serial/serial_core.c    | 25 +++++++++++++++++++++++++
 include/linux/serial_core.h         |  1 +
 3 files changed, 31 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 4440867b7d20..47c059987538 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -30,6 +30,7 @@
 #include <linux/uaccess.h>
 #include <linux/pm_runtime.h>
 #include <linux/ktime.h>
+#include <linux/gpio/consumer.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -681,6 +682,10 @@ int serial8250_em485_config(struct uart_port *port, struct serial_rs485 *rs485)
 	memset(rs485->padding, 0, sizeof(rs485->padding));
 	port->rs485 = *rs485;
 
+	if (port->rs485_term_gpio)
+		gpiod_set_value(port->rs485_term_gpio,
+				rs485->flags & SER_RS485_TERMINATE_BUS);
+
 	/*
 	 * Both serial8250_em485_init() and serial8250_em485_destroy()
 	 * are idempotent.
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 43b6682877d5..77702b6371e3 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -27,6 +27,7 @@
 
 #include <linux/irq.h>
 #include <linux/uaccess.h>
+#include <linux/gpio/consumer.h>
 
 /*
  * This is used to lock changes in serial line configuration.
@@ -3317,6 +3318,7 @@ int uart_get_rs485_mode(struct uart_port *port)
 	 * to get to a defined state with the following properties:
 	 */
 	rs485conf->flags &= ~(SER_RS485_RX_DURING_TX | SER_RS485_ENABLED |
+			      SER_RS485_TERMINATE_BUS |
 			      SER_RS485_RTS_AFTER_SEND);
 	rs485conf->flags |= SER_RS485_RTS_ON_SEND;
 
@@ -3331,6 +3333,29 @@ int uart_get_rs485_mode(struct uart_port *port)
 		rs485conf->flags |= SER_RS485_RTS_AFTER_SEND;
 	}
 
+	if (port->rs485_term_gpio)
+		devm_gpiod_put(dev, port->rs485_term_gpio);
+
+	port->rs485_term_gpio = devm_gpiod_get(dev, "rs485-term",
+					       GPIOD_FLAGS_BIT_DIR_OUT);
+	if (IS_ERR(port->rs485_term_gpio)) {
+		ret = PTR_ERR(port->rs485_term_gpio);
+		port->rs485_term_gpio = NULL;
+		if (ret != -ENOENT) {
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "Cannot get rs485-term-gpios\n");
+			return ret;
+		}
+	} else {
+		ret = gpiod_get_value(port->rs485_term_gpio);
+		if (ret < 0) {
+			dev_err(dev, "Cannot get rs485-term-gpios value\n");
+			return ret;
+		}
+		if (ret)
+			rs485conf->flags |= SER_RS485_TERMINATE_BUS;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(uart_get_rs485_mode);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index b649a2b894e7..9521e23b2144 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -251,6 +251,7 @@ struct uart_port {
 	struct attribute_group	*attr_group;		/* port specific attributes */
 	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
 	struct serial_rs485     rs485;
+	struct gpio_desc	*rs485_term_gpio;	/* enable RS485 bus termination */
 	struct serial_iso7816   iso7816;
 	void			*private_data;		/* generic platform data pointer */
 };
-- 
2.24.1

[PATCH v2 4/7] serial: 8250: Handle implementations not having TEMT interrupt using em485

[Heiko Stuebner]

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

Some 8250 ports have a TEMT interrupt but it's not a part of the 8250
standard, instead only available on some implementations.

The current em485 implementation does not work on ports without it.
The only chance to make it work is to loop-read on LSR register.

So add UART_CAP_TEMT to mark 8250 uarts having this interrupt,
update all current em485 users with that capability and make
the stop_tx function loop-read on uarts not having it.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
[moved to use added UART_CAP_TEMT, use readx_poll_timeout]
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 drivers/tty/serial/8250/8250.h            |  1 +
 drivers/tty/serial/8250/8250_bcm2835aux.c |  2 +-
 drivers/tty/serial/8250/8250_of.c         |  2 ++
 drivers/tty/serial/8250/8250_omap.c       |  2 +-
 drivers/tty/serial/8250/8250_port.c       | 25 +++++++++++++++++++----
 5 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 52bb21205bb6..770eb00db497 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -82,6 +82,7 @@ struct serial8250_config {
 #define UART_CAP_MINI	(1 << 17)	/* Mini UART on BCM283X family lacks:
 					 * STOP PARITY EPAR SPAR WLEN5 WLEN6
 					 */
+#define UART_CAP_TEMT	(1 << 18)	/* UART has TEMT interrupt */
 
 #define UART_BUG_QUOT	(1 << 0)	/* UART has buggy quot LSB */
 #define UART_BUG_TXEN	(1 << 1)	/* UART has buggy TX IIR status */
diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
index 12d03e678295..3881242424ca 100644
--- a/drivers/tty/serial/8250/8250_bcm2835aux.c
+++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
@@ -91,7 +91,7 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	/* initialize data */
-	up.capabilities = UART_CAP_FIFO | UART_CAP_MINI;
+	up.capabilities = UART_CAP_FIFO | UART_CAP_MINI | UART_CAP_TEMT;
 	up.port.dev = &pdev->dev;
 	up.port.regshift = 2;
 	up.port.type = PORT_16550;
diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
index 65e9045dafe6..841f6fcb2878 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -225,6 +225,8 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
 			&port8250.overrun_backoff_time_ms) != 0)
 		port8250.overrun_backoff_time_ms = 0;
 
+	port8250.capabilities |= UART_CAP_TEMT;
+
 	ret = serial8250_register_8250_port(&port8250);
 	if (ret < 0)
 		goto err_dispose;
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index dd69226ce918..d29d5b0cf8c1 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1140,7 +1140,7 @@ static int omap8250_probe(struct platform_device *pdev)
 	up.port.regshift = 2;
 	up.port.fifosize = 64;
 	up.tx_loadsz = 64;
-	up.capabilities = UART_CAP_FIFO;
+	up.capabilities = UART_CAP_FIFO | UART_CAP_TEMT;
 #ifdef CONFIG_PM
 	/*
 	 * Runtime PM is mostly transparent. However to do it right we need to a
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 47c059987538..41ad7db6a31e 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -15,6 +15,7 @@
 #include <linux/moduleparam.h>
 #include <linux/ioport.h>
 #include <linux/init.h>
+#include <linux/iopoll.h>
 #include <linux/console.h>
 #include <linux/sysrq.h>
 #include <linux/delay.h>
@@ -1520,6 +1521,11 @@ static inline void __do_stop_tx(struct uart_8250_port *p)
 		serial8250_rpm_put_tx(p);
 }
 
+static inline int __get_lsr(struct uart_8250_port *p)
+{
+	return serial_in(p, UART_LSR);
+}
+
 static inline void __stop_tx(struct uart_8250_port *p)
 {
 	struct uart_8250_em485 *em485 = p->em485;
@@ -1529,11 +1535,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 (p->capabilities & UART_CAP_TEMT) {
+			if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
+				return;
+		} else {
+			int lsr;
+
+			if (readx_poll_timeout(__get_lsr, p, lsr,
+					(lsr & BOTH_EMPTY) == BOTH_EMPTY,
+					0, 10000) < 0)
+				pr_warn("%s: timeout waiting for fifos to empty\n",
+					p->port.name);
+		}
 
 		__stop_tx_rs485(p);
 	}
-- 
2.24.1

[PATCH v2 5/7] dt-bindings: serial: Add binding for rs485 receiver enable GPIO

[Heiko Stuebner]

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

RS485 has two signals to control transmissions "drivee enable" (RE) and
"receiver enable" (DE). RE is already handled via the uarts RTS signal
while RE signal on most implementations doesn't get handled separately
at all.

As there still will be cases where this is needed though add a gpio
property for declaring this signal pin.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 Documentation/devicetree/bindings/serial/rs485.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/rs485.yaml b/Documentation/devicetree/bindings/serial/rs485.yaml
index a9ad17864889..e55730de796d 100644
--- a/Documentation/devicetree/bindings/serial/rs485.yaml
+++ b/Documentation/devicetree/bindings/serial/rs485.yaml
@@ -44,6 +44,10 @@ properties:
    description: enables the receiving of data even while sending data.
    $ref: /schemas/types.yaml#/definitions/flag
 
+  rs485-rx-enable-gpios:
+   description: GPIO to handle a separate RS485 receive enable signal
+   $ref: /schemas/types.yaml#/definitions/flag
+
   rs485-term-gpios:
     description: GPIO pin to enable RS485 bus termination.
     maxItems: 1
-- 
2.24.1

[PATCH v2 6/7] serial: 8250: Support separate rs485 rx-enable GPIO

[Heiko Stuebner]

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

The RE signal is used to control the duplex mode of transmissions,
aka receiving data while sending in full duplex mode, while stopping
receiving data in half-duplex mode.

On a number of boards the !RE signal is tied to ground so reception
is always enabled except if the UART allows disabling the receiver.
This can be taken advantage of to implement half-duplex mode - like
done on 8250_bcm2835aux.

Another solution is to tie !RE to RTS always forcing half-duplex mode.

And finally there is the option to control the RE signal separately,
like done here by introducing a new rs485-specifc gpio that can be
set depending on the RX_DURING_TX setting in the common em485 callbacks.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 drivers/tty/serial/8250/8250_port.c | 11 ++++++++++-
 drivers/tty/serial/serial_core.c    | 12 ++++++++++++
 include/linux/serial_core.h         |  1 +
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 41ad7db6a31e..6a34204126d9 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1445,6 +1445,7 @@ static void serial8250_stop_rx(struct uart_port *port)
 void serial8250_em485_stop_tx(struct uart_8250_port *p)
 {
 	unsigned char mcr = serial8250_in_MCR(p);
+	struct uart_port *port = &p->port;
 
 	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
 		mcr |= UART_MCR_RTS;
@@ -1458,6 +1459,9 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
 	 * Enable previously disabled RX interrupts.
 	 */
 	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
+		if (port->rs485_re_gpio)
+			gpiod_set_value(port->rs485_re_gpio, 1);
+
 		serial8250_clear_and_reinit_fifos(p);
 
 		p->ier |= UART_IER_RLSI | UART_IER_RDI;
@@ -1614,9 +1618,14 @@ static inline void __start_tx(struct uart_port *port)
 void serial8250_em485_start_tx(struct uart_8250_port *up)
 {
 	unsigned char mcr = serial8250_in_MCR(up);
+	struct uart_port *port = &up->port;
+
+	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
+		if (port->rs485_re_gpio)
+			gpiod_set_value(port->rs485_re_gpio, 0);
 
-	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
 		serial8250_stop_rx(&up->port);
+	}
 
 	if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
 		mcr |= UART_MCR_RTS;
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 77702b6371e3..5dc9ef31a6fa 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3356,6 +3356,18 @@ int uart_get_rs485_mode(struct uart_port *port)
 			rs485conf->flags |= SER_RS485_TERMINATE_BUS;
 	}
 
+	port->rs485_re_gpio = devm_gpiod_get(dev, "rs485-rx-enable",
+					       GPIOD_FLAGS_BIT_DIR_OUT);
+	if (IS_ERR(port->rs485_re_gpio)) {
+		ret = PTR_ERR(port->rs485_re_gpio);
+		port->rs485_re_gpio = NULL;
+		if (ret != -ENOENT) {
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "Cannot get rs485-rx-enable-gpios\n");
+			return ret;
+		}
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(uart_get_rs485_mode);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 9521e23b2144..4d8f51b1d9b2 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -252,6 +252,7 @@ struct uart_port {
 	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
 	struct serial_rs485     rs485;
 	struct gpio_desc	*rs485_term_gpio;	/* enable RS485 bus termination */
+	struct gpio_desc	*rs485_re_gpio;		/* gpio RS485 receive enable */
 	struct serial_iso7816   iso7816;
 	void			*private_data;		/* generic platform data pointer */
 };
-- 
2.24.1

[PATCH v2 7/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 and uses the standard em485 start and
stop helpers.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
[moved to use newly added start/stop helpers]
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 drivers/tty/serial/8250/8250_dw.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index aab3cccc6789..588319075b36 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -414,6 +414,9 @@ 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 = serial8250_em485_config;
+	up->rs485_start_tx = serial8250_em485_start_tx;
+	up->rs485_stop_tx = serial8250_em485_stop_tx;
 
 	p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
 	if (!p->membase)
-- 
2.24.1

Re: [PATCH v2 4/7] serial: 8250: Handle implementations not having TEMT interrupt using em485

[Giulio Benetti]

Hi Heiko,

very cleaner way to handle TEMT as a capability!
And I've found one thing...

Il 26/03/2020 00:14, Heiko Stuebner ha scritto:
> From: Giulio Benetti <giulio.benetti@micronovasrl.com>
> 
> Some 8250 ports have a TEMT interrupt but it's not a part of the 8250
> standard, instead only available on some implementations.
> 
> The current em485 implementation does not work on ports without it.
> The only chance to make it work is to loop-read on LSR register.
> 
> So add UART_CAP_TEMT to mark 8250 uarts having this interrupt,
> update all current em485 users with that capability and make
> the stop_tx function loop-read on uarts not having it.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> [moved to use added UART_CAP_TEMT, use readx_poll_timeout]
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> ---
>   drivers/tty/serial/8250/8250.h            |  1 +
>   drivers/tty/serial/8250/8250_bcm2835aux.c |  2 +-
>   drivers/tty/serial/8250/8250_of.c         |  2 ++
>   drivers/tty/serial/8250/8250_omap.c       |  2 +-
>   drivers/tty/serial/8250/8250_port.c       | 25 +++++++++++++++++++----
>   5 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index 52bb21205bb6..770eb00db497 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -82,6 +82,7 @@ struct serial8250_config {
>   #define UART_CAP_MINI	(1 << 17)	/* Mini UART on BCM283X family lacks:
>   					 * STOP PARITY EPAR SPAR WLEN5 WLEN6
>   					 */
> +#define UART_CAP_TEMT	(1 << 18)	/* UART has TEMT interrupt */
>   
>   #define UART_BUG_QUOT	(1 << 0)	/* UART has buggy quot LSB */
>   #define UART_BUG_TXEN	(1 << 1)	/* UART has buggy TX IIR status */
> diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
> index 12d03e678295..3881242424ca 100644
> --- a/drivers/tty/serial/8250/8250_bcm2835aux.c
> +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
> @@ -91,7 +91,7 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	/* initialize data */
> -	up.capabilities = UART_CAP_FIFO | UART_CAP_MINI;
> +	up.capabilities = UART_CAP_FIFO | UART_CAP_MINI | UART_CAP_TEMT;
>   	up.port.dev = &pdev->dev;
>   	up.port.regshift = 2;
>   	up.port.type = PORT_16550;
> diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
> index 65e9045dafe6..841f6fcb2878 100644
> --- a/drivers/tty/serial/8250/8250_of.c
> +++ b/drivers/tty/serial/8250/8250_of.c
> @@ -225,6 +225,8 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
>   			&port8250.overrun_backoff_time_ms) != 0)
>   		port8250.overrun_backoff_time_ms = 0;
>   
> +	port8250.capabilities |= UART_CAP_TEMT;
> +

Shouldn't this be NOT UART_CAP_TEMT set by default? On all other
vendor specific files you enable it, I think here you shouldn't enable
it too by default. Right?

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

>   	ret = serial8250_register_8250_port(&port8250);
>   	if (ret < 0)
>   		goto err_dispose;
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index dd69226ce918..d29d5b0cf8c1 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1140,7 +1140,7 @@ static int omap8250_probe(struct platform_device *pdev)
>   	up.port.regshift = 2;
>   	up.port.fifosize = 64;
>   	up.tx_loadsz = 64;
> -	up.capabilities = UART_CAP_FIFO;
> +	up.capabilities = UART_CAP_FIFO | UART_CAP_TEMT;
>   #ifdef CONFIG_PM
>   	/*
>   	 * Runtime PM is mostly transparent. However to do it right we need to a
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 47c059987538..41ad7db6a31e 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -15,6 +15,7 @@
>   #include <linux/moduleparam.h>
>   #include <linux/ioport.h>
>   #include <linux/init.h>
> +#include <linux/iopoll.h>
>   #include <linux/console.h>
>   #include <linux/sysrq.h>
>   #include <linux/delay.h>
> @@ -1520,6 +1521,11 @@ static inline void __do_stop_tx(struct uart_8250_port *p)
>   		serial8250_rpm_put_tx(p);
>   }
>   
> +static inline int __get_lsr(struct uart_8250_port *p)
> +{
> +	return serial_in(p, UART_LSR);
> +}
> +
>   static inline void __stop_tx(struct uart_8250_port *p)
>   {
>   	struct uart_8250_em485 *em485 = p->em485;
> @@ -1529,11 +1535,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 (p->capabilities & UART_CAP_TEMT) {
> +			if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
> +				return;
> +		} else {
> +			int lsr;
> +
> +			if (readx_poll_timeout(__get_lsr, p, lsr,
> +					(lsr & BOTH_EMPTY) == BOTH_EMPTY,
> +					0, 10000) < 0)
> +				pr_warn("%s: timeout waiting for fifos to empty\n",
> +					p->port.name);
> +		}
>   
>   		__stop_tx_rs485(p);
>   	}
> 

Re: [PATCH v2 4/7] serial: 8250: Handle implementations not having TEMT interrupt using em485

[Heiko Stübner]

Hi Giulio,

Am Donnerstag, 26. März 2020, 00:47:38 CET schrieb Giulio Benetti:
> very cleaner way to handle TEMT as a capability!
> And I've found one thing...
> 
> Il 26/03/2020 00:14, Heiko Stuebner ha scritto:
> > From: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > 
> > Some 8250 ports have a TEMT interrupt but it's not a part of the 8250
> > standard, instead only available on some implementations.
> > 
> > The current em485 implementation does not work on ports without it.
> > The only chance to make it work is to loop-read on LSR register.
> > 
> > So add UART_CAP_TEMT to mark 8250 uarts having this interrupt,
> > update all current em485 users with that capability and make
> > the stop_tx function loop-read on uarts not having it.
> > 
> > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > [moved to use added UART_CAP_TEMT, use readx_poll_timeout]
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > ---
> >   drivers/tty/serial/8250/8250.h            |  1 +
> >   drivers/tty/serial/8250/8250_bcm2835aux.c |  2 +-
> >   drivers/tty/serial/8250/8250_of.c         |  2 ++
> >   drivers/tty/serial/8250/8250_omap.c       |  2 +-
> >   drivers/tty/serial/8250/8250_port.c       | 25 +++++++++++++++++++----
> >   5 files changed, 26 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> > index 52bb21205bb6..770eb00db497 100644
> > --- a/drivers/tty/serial/8250/8250.h
> > +++ b/drivers/tty/serial/8250/8250.h
> > @@ -82,6 +82,7 @@ struct serial8250_config {
> >   #define UART_CAP_MINI	(1 << 17)	/* Mini UART on BCM283X family lacks:
> >   					 * STOP PARITY EPAR SPAR WLEN5 WLEN6
> >   					 */
> > +#define UART_CAP_TEMT	(1 << 18)	/* UART has TEMT interrupt */
> >   
> >   #define UART_BUG_QUOT	(1 << 0)	/* UART has buggy quot LSB */
> >   #define UART_BUG_TXEN	(1 << 1)	/* UART has buggy TX IIR status */
> > diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
> > index 12d03e678295..3881242424ca 100644
> > --- a/drivers/tty/serial/8250/8250_bcm2835aux.c
> > +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
> > @@ -91,7 +91,7 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
> >   		return -ENOMEM;
> >   
> >   	/* initialize data */
> > -	up.capabilities = UART_CAP_FIFO | UART_CAP_MINI;
> > +	up.capabilities = UART_CAP_FIFO | UART_CAP_MINI | UART_CAP_TEMT;
> >   	up.port.dev = &pdev->dev;
> >   	up.port.regshift = 2;
> >   	up.port.type = PORT_16550;
> > diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
> > index 65e9045dafe6..841f6fcb2878 100644
> > --- a/drivers/tty/serial/8250/8250_of.c
> > +++ b/drivers/tty/serial/8250/8250_of.c
> > @@ -225,6 +225,8 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
> >   			&port8250.overrun_backoff_time_ms) != 0)
> >   		port8250.overrun_backoff_time_ms = 0;
> >   
> > +	port8250.capabilities |= UART_CAP_TEMT;
> > +
> 
> Shouldn't this be NOT UART_CAP_TEMT set by default? On all other
> vendor specific files you enable it, I think here you shouldn't enable
> it too by default. Right?

8250_of does use the em485 emulation - see of_platform_serial_setup()
So I did go by the lazy assumption that any 8250 driver using rs485
before my series always used the interrupt driver code path, so
implicitly required to have the TEMT interrupt.

Of course, you're right that with the 8250_of maybe not all variants
actually do have this interrupt, so falling back to the polling here might
be safer.


Heiko

Re: [PATCH v2 4/7] serial: 8250: Handle implementations not having TEMT interrupt using em485

[Giulio Benetti]

Hi Heiko,

Il 26/03/2020 01:05, Heiko Stübner ha scritto:
> Hi Giulio,
> 
> Am Donnerstag, 26. März 2020, 00:47:38 CET schrieb Giulio Benetti:
>> very cleaner way to handle TEMT as a capability!
>> And I've found one thing...
>>
>> Il 26/03/2020 00:14, Heiko Stuebner ha scritto:
>>> From: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>
>>> Some 8250 ports have a TEMT interrupt but it's not a part of the 8250
>>> standard, instead only available on some implementations.
>>>
>>> The current em485 implementation does not work on ports without it.
>>> The only chance to make it work is to loop-read on LSR register.
>>>
>>> So add UART_CAP_TEMT to mark 8250 uarts having this interrupt,
>>> update all current em485 users with that capability and make
>>> the stop_tx function loop-read on uarts not having it.
>>>
>>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>> [moved to use added UART_CAP_TEMT, use readx_poll_timeout]
>>> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
>>> ---
>>>    drivers/tty/serial/8250/8250.h            |  1 +
>>>    drivers/tty/serial/8250/8250_bcm2835aux.c |  2 +-
>>>    drivers/tty/serial/8250/8250_of.c         |  2 ++
>>>    drivers/tty/serial/8250/8250_omap.c       |  2 +-
>>>    drivers/tty/serial/8250/8250_port.c       | 25 +++++++++++++++++++----
>>>    5 files changed, 26 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>>> index 52bb21205bb6..770eb00db497 100644
>>> --- a/drivers/tty/serial/8250/8250.h
>>> +++ b/drivers/tty/serial/8250/8250.h
>>> @@ -82,6 +82,7 @@ struct serial8250_config {
>>>    #define UART_CAP_MINI	(1 << 17)	/* Mini UART on BCM283X family lacks:
>>>    					 * STOP PARITY EPAR SPAR WLEN5 WLEN6
>>>    					 */
>>> +#define UART_CAP_TEMT	(1 << 18)	/* UART has TEMT interrupt */
>>>    
>>>    #define UART_BUG_QUOT	(1 << 0)	/* UART has buggy quot LSB */
>>>    #define UART_BUG_TXEN	(1 << 1)	/* UART has buggy TX IIR status */
>>> diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
>>> index 12d03e678295..3881242424ca 100644
>>> --- a/drivers/tty/serial/8250/8250_bcm2835aux.c
>>> +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
>>> @@ -91,7 +91,7 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
>>>    		return -ENOMEM;
>>>    
>>>    	/* initialize data */
>>> -	up.capabilities = UART_CAP_FIFO | UART_CAP_MINI;
>>> +	up.capabilities = UART_CAP_FIFO | UART_CAP_MINI | UART_CAP_TEMT;
>>>    	up.port.dev = &pdev->dev;
>>>    	up.port.regshift = 2;
>>>    	up.port.type = PORT_16550;
>>> diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
>>> index 65e9045dafe6..841f6fcb2878 100644
>>> --- a/drivers/tty/serial/8250/8250_of.c
>>> +++ b/drivers/tty/serial/8250/8250_of.c
>>> @@ -225,6 +225,8 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
>>>    			&port8250.overrun_backoff_time_ms) != 0)
>>>    		port8250.overrun_backoff_time_ms = 0;
>>>    
>>> +	port8250.capabilities |= UART_CAP_TEMT;
>>> +
>>
>> Shouldn't this be NOT UART_CAP_TEMT set by default? On all other
>> vendor specific files you enable it, I think here you shouldn't enable
>> it too by default. Right?
> 
> 8250_of does use the em485 emulation - see of_platform_serial_setup()
> So I did go by the lazy assumption that any 8250 driver using rs485
> before my series always used the interrupt driver code path, so
> implicitly required to have the TEMT interrupt.
> 
> Of course, you're right that with the 8250_of maybe not all variants
> actually do have this interrupt, so falling back to the polling here might
> be safer.

Probably here it's worth introducing a dt boolean property like
"temt-capability", then you set or not UART_CAP_TEMT according to its 
presence in dts. This way all cases are covered and we can act 
completely through dts files.

What about that?

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

Re: [PATCH DON'T APPLY v2 3/7] serial: 8250: Support rs485 bus termination GPIO

[Andy Shevchenko]

On Thu, Mar 26, 2020 at 12:14:18AM +0100, Heiko Stuebner wrote:
> From: Lukas Wunner <lukas@wunner.de>
> 
> Amend the serial core to retrieve the rs485 bus termination GPIO from
> the device tree (or ACPI table) and amend the default ->rs485_config()
> callback for 8250 drivers to change the GPIO on request from user space.

> +	port->rs485_term_gpio = devm_gpiod_get(dev, "rs485-term",
> +					       GPIOD_FLAGS_BIT_DIR_OUT);
> +	if (IS_ERR(port->rs485_term_gpio)) {
> +		ret = PTR_ERR(port->rs485_term_gpio);
> +		port->rs485_term_gpio = NULL;
> +		if (ret != -ENOENT) {
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "Cannot get rs485-term-gpios\n");
> +			return ret;
> +		}

NIH of gpiod_get_optional().

> +	} else {
> +		ret = gpiod_get_value(port->rs485_term_gpio);
> +		if (ret < 0) {
> +			dev_err(dev, "Cannot get rs485-term-gpios value\n");
> +			return ret;
> +		}
> +		if (ret)
> +			rs485conf->flags |= SER_RS485_TERMINATE_BUS;
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/7] serial: 8250: Handle implementations not having TEMT interrupt using em485

[Andy Shevchenko]

On Thu, Mar 26, 2020 at 12:14:19AM +0100, Heiko Stuebner wrote:
> From: Giulio Benetti <giulio.benetti@micronovasrl.com>
> 
> Some 8250 ports have a TEMT interrupt but it's not a part of the 8250
> standard, instead only available on some implementations.
> 
> The current em485 implementation does not work on ports without it.
> The only chance to make it work is to loop-read on LSR register.
> 
> So add UART_CAP_TEMT to mark 8250 uarts having this interrupt,
> update all current em485 users with that capability and make
> the stop_tx function loop-read on uarts not having it.

> +		if (p->capabilities & UART_CAP_TEMT) {
> +			if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
> +				return;
> +		} else {
> +			int lsr;
> +

> +			if (readx_poll_timeout(__get_lsr, p, lsr,
> +					(lsr & BOTH_EMPTY) == BOTH_EMPTY,
> +					0, 10000) < 0)

			ret = readx_poll_timeout(...);
			if (ret)
				...

will look better.

> +				pr_warn("%s: timeout waiting for fifos to empty\n",
> +					p->port.name);
> +		}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/7] dt-bindings: serial: Add binding for rs485 receiver enable GPIO

[Andy Shevchenko]

On Thu, Mar 26, 2020 at 12:14:20AM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> RS485 has two signals to control transmissions "drivee enable" (RE) and

drivee -> driver ?

> "receiver enable" (DE). RE is already handled via the uarts RTS signal

Typo in abbreviations in parentheses, you probably need to swap them.

> while RE signal on most implementations doesn't get handled separately
> at all.
> 
> As there still will be cases where this is needed though add a gpio
> property for declaring this signal pin.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> ---
>  Documentation/devicetree/bindings/serial/rs485.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/rs485.yaml b/Documentation/devicetree/bindings/serial/rs485.yaml
> index a9ad17864889..e55730de796d 100644
> --- a/Documentation/devicetree/bindings/serial/rs485.yaml
> +++ b/Documentation/devicetree/bindings/serial/rs485.yaml
> @@ -44,6 +44,10 @@ properties:
>     description: enables the receiving of data even while sending data.
>     $ref: /schemas/types.yaml#/definitions/flag
>  
> +  rs485-rx-enable-gpios:
> +   description: GPIO to handle a separate RS485 receive enable signal
> +   $ref: /schemas/types.yaml#/definitions/flag
> +
>    rs485-term-gpios:
>      description: GPIO pin to enable RS485 bus termination.
>      maxItems: 1
> -- 
> 2.24.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/7] serial: 8250: Support separate rs485 rx-enable GPIO

[Andy Shevchenko]

On Thu, Mar 26, 2020 at 12:14:21AM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> The RE signal is used to control the duplex mode of transmissions,
> aka receiving data while sending in full duplex mode, while stopping
> receiving data in half-duplex mode.
> 
> On a number of boards the !RE signal is tied to ground so reception
> is always enabled except if the UART allows disabling the receiver.
> This can be taken advantage of to implement half-duplex mode - like
> done on 8250_bcm2835aux.
> 
> Another solution is to tie !RE to RTS always forcing half-duplex mode.
> 
> And finally there is the option to control the RE signal separately,
> like done here by introducing a new rs485-specifc gpio that can be
> set depending on the RX_DURING_TX setting in the common em485 callbacks.


> +		if (port->rs485_re_gpio)

Redundant. Same for all other cases.

> +			gpiod_set_value(port->rs485_re_gpio, 1);


> +	port->rs485_re_gpio = devm_gpiod_get(dev, "rs485-rx-enable",
> +					       GPIOD_FLAGS_BIT_DIR_OUT);
> +	if (IS_ERR(port->rs485_re_gpio)) {
> +		ret = PTR_ERR(port->rs485_re_gpio);
> +		port->rs485_re_gpio = NULL;
> +		if (ret != -ENOENT) {
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "Cannot get rs485-rx-enable-gpios\n");
> +			return ret;
> +		}
> +	}

NIH of gpiod_get_optional().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/7] serial: 8250: Handle implementations not having TEMT interrupt using em485

[Lukas Wunner]

On Thu, Mar 26, 2020 at 12:14:19AM +0100, Heiko Stuebner wrote:
> Some 8250 ports have a TEMT interrupt but it's not a part of the 8250
> standard, instead only available on some implementations.
> 
> The current em485 implementation does not work on ports without it.
> The only chance to make it work is to loop-read on LSR register.
> 
> So add UART_CAP_TEMT to mark 8250 uarts having this interrupt,
> update all current em485 users with that capability and make
> the stop_tx function loop-read on uarts not having it.

Just to get a better understanding:  According to the Dw_apb_uart_db.pdf
databook I've found, the UART does have a "THR empty" interrupt.  So you
get an interrupt once the Transmit Holding Register (and by consequence
the FIFO) has been drained.  Then what do you need a TEMT interrupt for?
Why is the THR interrupt not sufficient?


> @@ -1529,11 +1535,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 (p->capabilities & UART_CAP_TEMT) {
> +			if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
> +				return;
> +		} else {
> +			int lsr;
> +
> +			if (readx_poll_timeout(__get_lsr, p, lsr,
> +					(lsr & BOTH_EMPTY) == BOTH_EMPTY,
> +					0, 10000) < 0)
> +				pr_warn("%s: timeout waiting for fifos to empty\n",
> +					p->port.name);
> +		}

Do you actually need to check for the timeout?  How could this happen?
Only if some other part of the driver would disable the transmitter
I guess, which would be a bug.

Also, note that __stop_tx() may be called from hardirq context via
serial8250_tx_chars().  If the baudrate is low, you may spin for a
fairly long time in IRQ context.  E.g. with 9600 8N1, it takes about
1 msec for one char to transmit.

Thanks,

Lukas

Re: [PATCH v2 5/7] dt-bindings: serial: Add binding for rs485 receiver enable GPIO

[Lukas Wunner]

On Thu, Mar 26, 2020 at 12:14:20AM +0100, Heiko Stuebner wrote:
> --- a/Documentation/devicetree/bindings/serial/rs485.yaml
> +++ b/Documentation/devicetree/bindings/serial/rs485.yaml
> @@ -44,6 +44,10 @@ properties:
>     description: enables the receiving of data even while sending data.
>     $ref: /schemas/types.yaml#/definitions/flag
>  
> +  rs485-rx-enable-gpios:
> +   description: GPIO to handle a separate RS485 receive enable signal
> +   $ref: /schemas/types.yaml#/definitions/flag
> +

This isn't a flag, so you need the "maxItems: 1" line instead,
as you correctly did below:

>    rs485-term-gpios:
>      description: GPIO pin to enable RS485 bus termination.
>      maxItems: 1

Re: [PATCH v2 4/7] serial: 8250: Handle implementations not having TEMT interrupt using em485

[Maarten Brock]

On 2020-05-02 15:49, Lukas Wunner wrote:
> On Thu, Mar 26, 2020 at 12:14:19AM +0100, Heiko Stuebner wrote:
>> Some 8250 ports have a TEMT interrupt but it's not a part of the 8250
>> standard, instead only available on some implementations.
>> 
>> The current em485 implementation does not work on ports without it.
>> The only chance to make it work is to loop-read on LSR register.
>> 
>> So add UART_CAP_TEMT to mark 8250 uarts having this interrupt,
>> update all current em485 users with that capability and make
>> the stop_tx function loop-read on uarts not having it.
> 
> Just to get a better understanding:  According to the 
> Dw_apb_uart_db.pdf
> databook I've found, the UART does have a "THR empty" interrupt.  So 
> you
> get an interrupt once the Transmit Holding Register (and by consequence
> the FIFO) has been drained.  Then what do you need a TEMT interrupt 
> for?
> Why is the THR interrupt not sufficient?

When the Transmit Holding Register is empty, the Transmitter can still 
be
transmitting. And the Driver Enable must be held active during 
transmission.
I would even say it needs to held active during transmission of the stop 
bit,
but I don't believe there is any uart that has an interrupt flag for 
that.
And since the default state for RS485 is '1' anyway it's not that bad.

Maarten

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

[Andy Shevchenko]

On Thu, Mar 26, 2020 at 1:17 AM Heiko Stuebner <heiko@sntech.de> wrote:

If it's not covered by either yours or Lukas' series, perhaps worth to
address as well.

.../8250_port.c:1427: warning: Function parameter or member 'p
' not described in 'serial8250_em485_stop_tx'
.../8250_port.c:1427: warning: Excess function parameter 'up'
description in 'serial8250_em485_stop_tx'

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/7] serial: 8250: Support separate rs485 rx-enable GPIO

[Lukas Wunner]

On Thu, Mar 26, 2020 at 12:14:21AM +0100, Heiko Stuebner wrote:
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3356,6 +3356,18 @@ int uart_get_rs485_mode(struct uart_port *port)
>  			rs485conf->flags |= SER_RS485_TERMINATE_BUS;
>  	}
>  
> +	port->rs485_re_gpio = devm_gpiod_get(dev, "rs485-rx-enable",
> +					       GPIOD_FLAGS_BIT_DIR_OUT);

I think you want to use GPIOD_OUT_HIGH here so that rs485 reception
is enabled by default and only disabled at runtime when sending
(if half-duplex mode is used).

Thanks,

Lukas

Re: [PATCH v2 4/7] serial: 8250: Handle implementations not having TEMT interrupt using em485

[Heiko Stübner]

Hi Giulio,

Am Donnerstag, 26. März 2020, 03:02:39 CEST schrieb Giulio Benetti:
> Il 26/03/2020 01:05, Heiko Stübner ha scritto:
> > Am Donnerstag, 26. März 2020, 00:47:38 CET schrieb Giulio Benetti:
> >> very cleaner way to handle TEMT as a capability!
> >> And I've found one thing...
> >>
> >> Il 26/03/2020 00:14, Heiko Stuebner ha scritto:
> >>> From: Giulio Benetti <giulio.benetti@micronovasrl.com>
> >>>
> >>> Some 8250 ports have a TEMT interrupt but it's not a part of the 8250
> >>> standard, instead only available on some implementations.
> >>>
> >>> The current em485 implementation does not work on ports without it.
> >>> The only chance to make it work is to loop-read on LSR register.
> >>>
> >>> So add UART_CAP_TEMT to mark 8250 uarts having this interrupt,
> >>> update all current em485 users with that capability and make
> >>> the stop_tx function loop-read on uarts not having it.
> >>>
> >>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> >>> [moved to use added UART_CAP_TEMT, use readx_poll_timeout]
> >>> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> >>> ---
> >>>    drivers/tty/serial/8250/8250.h            |  1 +
> >>>    drivers/tty/serial/8250/8250_bcm2835aux.c |  2 +-
> >>>    drivers/tty/serial/8250/8250_of.c         |  2 ++
> >>>    drivers/tty/serial/8250/8250_omap.c       |  2 +-
> >>>    drivers/tty/serial/8250/8250_port.c       | 25 +++++++++++++++++++----
> >>>    5 files changed, 26 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> >>> index 52bb21205bb6..770eb00db497 100644
> >>> --- a/drivers/tty/serial/8250/8250.h
> >>> +++ b/drivers/tty/serial/8250/8250.h
> >>> @@ -82,6 +82,7 @@ struct serial8250_config {
> >>>    #define UART_CAP_MINI	(1 << 17)	/* Mini UART on BCM283X family lacks:
> >>>    					 * STOP PARITY EPAR SPAR WLEN5 WLEN6
> >>>    					 */
> >>> +#define UART_CAP_TEMT	(1 << 18)	/* UART has TEMT interrupt */
> >>>    
> >>>    #define UART_BUG_QUOT	(1 << 0)	/* UART has buggy quot LSB */
> >>>    #define UART_BUG_TXEN	(1 << 1)	/* UART has buggy TX IIR status */
> >>> diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
> >>> index 12d03e678295..3881242424ca 100644
> >>> --- a/drivers/tty/serial/8250/8250_bcm2835aux.c
> >>> +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
> >>> @@ -91,7 +91,7 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
> >>>    		return -ENOMEM;
> >>>    
> >>>    	/* initialize data */
> >>> -	up.capabilities = UART_CAP_FIFO | UART_CAP_MINI;
> >>> +	up.capabilities = UART_CAP_FIFO | UART_CAP_MINI | UART_CAP_TEMT;
> >>>    	up.port.dev = &pdev->dev;
> >>>    	up.port.regshift = 2;
> >>>    	up.port.type = PORT_16550;
> >>> diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
> >>> index 65e9045dafe6..841f6fcb2878 100644
> >>> --- a/drivers/tty/serial/8250/8250_of.c
> >>> +++ b/drivers/tty/serial/8250/8250_of.c
> >>> @@ -225,6 +225,8 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
> >>>    			&port8250.overrun_backoff_time_ms) != 0)
> >>>    		port8250.overrun_backoff_time_ms = 0;
> >>>    
> >>> +	port8250.capabilities |= UART_CAP_TEMT;
> >>> +
> >>
> >> Shouldn't this be NOT UART_CAP_TEMT set by default? On all other
> >> vendor specific files you enable it, I think here you shouldn't enable
> >> it too by default. Right?
> > 
> > 8250_of does use the em485 emulation - see of_platform_serial_setup()
> > So I did go by the lazy assumption that any 8250 driver using rs485
> > before my series always used the interrupt driver code path, so
> > implicitly required to have the TEMT interrupt.
> > 
> > Of course, you're right that with the 8250_of maybe not all variants
> > actually do have this interrupt, so falling back to the polling here might
> > be safer.
> 
> Probably here it's worth introducing a dt boolean property like
> "temt-capability", then you set or not UART_CAP_TEMT according to its 
> presence in dts. This way all cases are covered and we can act 
> completely through dts files.
> 
> What about that?

Sorry that this was sitting around for over a month.

I think there are two problems with this:

(1) this would break backwards compatibility ... right now the whole code
just assumes that everyone does support the TEMT interrupt, so adding
a property to keep it working would break old DTs, which is something that
should not happen ... I guess one option would be to use the inverse
no-temt-interrupt

(2) uarts handled by 8250_of are still identified by their compatible
though and there is no generic 8250-of compatible, so the
presence / absence of the temt capability should actually just be
bound to the relevant compatible.


So my "gut feeling" is to just keep the current way
(was expecting temt-capability before anyway) until an uart
variant without temt comes along


Heiko

Re: [PATCH v2 4/7] serial: 8250: Handle implementations not having TEMT interrupt using em485

[Giulio Benetti]

Hi Heiko,

Il 17/05/2020 17:04, Heiko Stübner ha scritto:
> Hi Giulio,
> 
> Am Donnerstag, 26. März 2020, 03:02:39 CEST schrieb Giulio Benetti:
>> Il 26/03/2020 01:05, Heiko Stübner ha scritto:
>>> Am Donnerstag, 26. März 2020, 00:47:38 CET schrieb Giulio Benetti:
>>>> very cleaner way to handle TEMT as a capability!
>>>> And I've found one thing...
>>>>
>>>> Il 26/03/2020 00:14, Heiko Stuebner ha scritto:
>>>>> From: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>>>
>>>>> Some 8250 ports have a TEMT interrupt but it's not a part of the 8250
>>>>> standard, instead only available on some implementations.
>>>>>
>>>>> The current em485 implementation does not work on ports without it.
>>>>> The only chance to make it work is to loop-read on LSR register.
>>>>>
>>>>> So add UART_CAP_TEMT to mark 8250 uarts having this interrupt,
>>>>> update all current em485 users with that capability and make
>>>>> the stop_tx function loop-read on uarts not having it.
>>>>>
>>>>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>>> [moved to use added UART_CAP_TEMT, use readx_poll_timeout]
>>>>> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
>>>>> ---
>>>>>     drivers/tty/serial/8250/8250.h            |  1 +
>>>>>     drivers/tty/serial/8250/8250_bcm2835aux.c |  2 +-
>>>>>     drivers/tty/serial/8250/8250_of.c         |  2 ++
>>>>>     drivers/tty/serial/8250/8250_omap.c       |  2 +-
>>>>>     drivers/tty/serial/8250/8250_port.c       | 25 +++++++++++++++++++----
>>>>>     5 files changed, 26 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>>>>> index 52bb21205bb6..770eb00db497 100644
>>>>> --- a/drivers/tty/serial/8250/8250.h
>>>>> +++ b/drivers/tty/serial/8250/8250.h
>>>>> @@ -82,6 +82,7 @@ struct serial8250_config {
>>>>>     #define UART_CAP_MINI	(1 << 17)	/* Mini UART on BCM283X family lacks:
>>>>>     					 * STOP PARITY EPAR SPAR WLEN5 WLEN6
>>>>>     					 */
>>>>> +#define UART_CAP_TEMT	(1 << 18)	/* UART has TEMT interrupt */
>>>>>     
>>>>>     #define UART_BUG_QUOT	(1 << 0)	/* UART has buggy quot LSB */
>>>>>     #define UART_BUG_TXEN	(1 << 1)	/* UART has buggy TX IIR status */
>>>>> diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
>>>>> index 12d03e678295..3881242424ca 100644
>>>>> --- a/drivers/tty/serial/8250/8250_bcm2835aux.c
>>>>> +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
>>>>> @@ -91,7 +91,7 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
>>>>>     		return -ENOMEM;
>>>>>     
>>>>>     	/* initialize data */
>>>>> -	up.capabilities = UART_CAP_FIFO | UART_CAP_MINI;
>>>>> +	up.capabilities = UART_CAP_FIFO | UART_CAP_MINI | UART_CAP_TEMT;
>>>>>     	up.port.dev = &pdev->dev;
>>>>>     	up.port.regshift = 2;
>>>>>     	up.port.type = PORT_16550;
>>>>> diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
>>>>> index 65e9045dafe6..841f6fcb2878 100644
>>>>> --- a/drivers/tty/serial/8250/8250_of.c
>>>>> +++ b/drivers/tty/serial/8250/8250_of.c
>>>>> @@ -225,6 +225,8 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
>>>>>     			&port8250.overrun_backoff_time_ms) != 0)
>>>>>     		port8250.overrun_backoff_time_ms = 0;
>>>>>     
>>>>> +	port8250.capabilities |= UART_CAP_TEMT;
>>>>> +
>>>>
>>>> Shouldn't this be NOT UART_CAP_TEMT set by default? On all other
>>>> vendor specific files you enable it, I think here you shouldn't enable
>>>> it too by default. Right?
>>>
>>> 8250_of does use the em485 emulation - see of_platform_serial_setup()
>>> So I did go by the lazy assumption that any 8250 driver using rs485
>>> before my series always used the interrupt driver code path, so
>>> implicitly required to have the TEMT interrupt.
>>>
>>> Of course, you're right that with the 8250_of maybe not all variants
>>> actually do have this interrupt, so falling back to the polling here might
>>> be safer.
>>
>> Probably here it's worth introducing a dt boolean property like
>> "temt-capability", then you set or not UART_CAP_TEMT according to its
>> presence in dts. This way all cases are covered and we can act
>> completely through dts files.
>>
>> What about that?
> 
> Sorry that this was sitting around for over a month.

np at all

> I think there are two problems with this:
> 
> (1) this would break backwards compatibility ... right now the whole code
> just assumes that everyone does support the TEMT interrupt, so adding
> a property to keep it working would break old DTs, which is something that
> should not happen ... I guess one option would be to use the inverse
> no-temt-interrupt
> 
> (2) uarts handled by 8250_of are still identified by their compatible
> though and there is no generic 8250-of compatible, so the
> presence / absence of the temt capability should actually just be
> bound to the relevant compatible.
> 
> 
> So my "gut feeling" is to just keep the current way
> (was expecting temt-capability before anyway) until an uart
> variant without temt comes along

I agree with you, what I was proposing is another thing more to do an
can be done when needed.

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

Re: [PATCH v2 4/7] serial: 8250: Handle implementations not having TEMT interrupt using em485

[Heiko Stübner]

Hi Lukas,

Am Samstag, 2. Mai 2020, 15:49:27 CEST schrieb Lukas Wunner:
> On Thu, Mar 26, 2020 at 12:14:19AM +0100, Heiko Stuebner wrote:
> > @@ -1529,11 +1535,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 (p->capabilities & UART_CAP_TEMT) {
> > +			if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
> > +				return;
> > +		} else {
> > +			int lsr;
> > +
> > +			if (readx_poll_timeout(__get_lsr, p, lsr,
> > +					(lsr & BOTH_EMPTY) == BOTH_EMPTY,
> > +					0, 10000) < 0)
> > +				pr_warn("%s: timeout waiting for fifos to empty\n",
> > +					p->port.name);
> > +		}
> 
> Do you actually need to check for the timeout?  How could this happen?
> Only if some other part of the driver would disable the transmitter
> I guess, which would be a bug.

Checking for a timeout was strongly suggested in v1 ;-)


> Also, note that __stop_tx() may be called from hardirq context via
> serial8250_tx_chars().  If the baudrate is low, you may spin for a
> fairly long time in IRQ context.  E.g. with 9600 8N1, it takes about
> 1 msec for one char to transmit.

I did play around with different baud rates and data amounts today
and even ran into the timeout with the current 10ms when doing a
"dmesg > /dev/ttyS3" ... combined with the hardirq issue you mentioned
I think I found a slightly better variant to do this ... by catching the first
100us in the interrupt handler and otherwise re-using the existing
stop-timer infrastructure to move this out of the actual __stop_tx function.

I've sent a v3 based on your new series just now ... if you find time
please have a look :-)

Thanks
Heiko