[PATCH v3 0/5] tty: Introduce software RS485 direction control support

[PATCH v3 0/5] tty: Introduce software RS485 direction control support

[Matwey V. Kornilov]

Changes from v2:
 - Introduced SER_RS485_SOFTWARE to show that software implementation is being used
 - serial8250_rs485_config is located as required
 - Timers are used to implement delay_before and delay_after timeouts

[PATCH v3 1/5] tty: Introduce UART_CAP_HW485

[Matwey V. Kornilov]

Introduce new capability UART_CAP_HW485 to mark 8250 UARTs which have
hardware support of line direction control.

Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
 drivers/tty/serial/8250/8250.h         | 1 +
 drivers/tty/serial/8250/8250_fintek.c  | 1 +
 drivers/tty/serial/8250/8250_lpc18xx.c | 1 +
 drivers/tty/serial/8250/8250_pci.c     | 1 +
 4 files changed, 4 insertions(+)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index d54dcd8..92a4f47 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -69,6 +69,7 @@ struct serial8250_config {
 	unsigned int	flags;
 };
 
+#define UART_CAP_HW485	(1 << 7)	/* UART has hardware direction control for RS485 */
 #define UART_CAP_FIFO	(1 << 8)	/* UART has FIFO */
 #define UART_CAP_EFR	(1 << 9)	/* UART has EFR */
 #define UART_CAP_SLEEP	(1 << 10)	/* UART has IER sleep */
diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c
index 8947439..f2831a8 100644
--- a/drivers/tty/serial/8250/8250_fintek.c
+++ b/drivers/tty/serial/8250/8250_fintek.c
@@ -208,6 +208,7 @@ fintek_8250_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
 	uart.port.iobase = pnp_port_start(dev, 0);
 	uart.port.iotype = UPIO_PORT;
 	uart.port.rs485_config = fintek_8250_rs485_config;
+	uart.capabilities = UART_CAP_HW485;
 
 	uart.port.flags |= UPF_SKIP_TEST | UPF_BOOT_AUTOCONF;
 	if (pnp_irq_flags(dev, 0) & IORESOURCE_IRQ_SHAREABLE)
diff --git a/drivers/tty/serial/8250/8250_lpc18xx.c b/drivers/tty/serial/8250/8250_lpc18xx.c
index 99cd478..9d30276 100644
--- a/drivers/tty/serial/8250/8250_lpc18xx.c
+++ b/drivers/tty/serial/8250/8250_lpc18xx.c
@@ -175,6 +175,7 @@ static int lpc18xx_serial_probe(struct platform_device *pdev)
 	uart.port.private_data = data;
 	uart.port.rs485_config = lpc18xx_rs485_config;
 	uart.port.serial_out = lpc18xx_uart_serial_out;
+	uart.capabilities = UART_CAP_HW485;
 
 	uart.dma = &data->dma;
 	uart.dma->rxconf.src_maxburst = 1;
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 4097f3f..c7c0ae9 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -1599,6 +1599,7 @@ static int pci_fintek_setup(struct serial_private *priv,
 	port->port.iotype = UPIO_PORT;
 	port->port.iobase = iobase;
 	port->port.rs485_config = pci_fintek_rs485_config;
+	port->capabilities = UART_CAP_HW485;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(u8), GFP_KERNEL);
 	if (!data)
-- 
2.6.2

[PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Matwey V. Kornilov]

This flag is supposed to be used by uart drivers using software rs485 direction control.

Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
 include/uapi/linux/serial.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
index 25331f9..95b15ca 100644
--- a/include/uapi/linux/serial.h
+++ b/include/uapi/linux/serial.h
@@ -121,6 +121,9 @@ struct serial_rs485 {
 #define SER_RS485_RTS_AFTER_SEND	(1 << 2)	/* Logical level for
 							   RTS pin after sent*/
 #define SER_RS485_RX_DURING_TX		(1 << 4)
+#define SER_RS485_SOFTWARE		(1 << 5)	/* Software
+							   implementation is
+							   being used */
 	__u32	delay_rts_before_send;	/* Delay before send (milliseconds) */
 	__u32	delay_rts_after_send;	/* Delay after send (milliseconds) */
 	__u32	padding[5];		/* Memory is cheap, new structs
-- 
2.6.2

[PATCH v3 3/5] tty: Implement default fallback serial8250_rs485_config

[Matwey V. Kornilov]

When 8250 driver doesn't have its own hardware RS485 support and doesn't
want to override rs485_config callback, then default
serial8250_rs485_config is used. It just stores supplied by user-space
config and sets SER_RS485_SOFTWARE

Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
 drivers/tty/serial/8250/8250_core.c |  3 ++-
 drivers/tty/serial/8250/8250_port.c | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 3912646..71fabb0 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -986,7 +986,6 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 		uart->capabilities	= up->capabilities;
 		uart->port.throttle	= up->port.throttle;
 		uart->port.unthrottle	= up->port.unthrottle;
-		uart->port.rs485_config	= up->port.rs485_config;
 		uart->port.rs485	= up->port.rs485;
 		uart->dma		= up->dma;
 
@@ -1025,6 +1024,8 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 			uart->port.pm = up->port.pm;
 		if (up->port.handle_break)
 			uart->port.handle_break = up->port.handle_break;
+		if (up->port.rs485_config)
+			uart->port.rs485_config	= up->port.rs485_config;
 		if (up->dl_read)
 			uart->dl_read = up->dl_read;
 		if (up->dl_write)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 52d82d2..067ef55 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2735,6 +2735,14 @@ serial8250_type(struct uart_port *port)
 	return uart_config[type].name;
 }
 
+static int serial8250_rs485_config(struct uart_port *port,
+				struct serial_rs485 *rs485)
+{
+	port->rs485 = *rs485;
+	port->rs485.flags |= SER_RS485_SOFTWARE;
+	return 0;
+}
+
 static const struct uart_ops serial8250_pops = {
 	.tx_empty	= serial8250_tx_empty,
 	.set_mctrl	= serial8250_set_mctrl,
@@ -2797,6 +2805,8 @@ void serial8250_set_defaults(struct uart_8250_port *up)
 		if (!up->dma->rx_dma)
 			up->dma->rx_dma = serial8250_rx_dma;
 	}
+
+	up->port.rs485_config = serial8250_rs485_config;
 }
 EXPORT_SYMBOL_GPL(serial8250_set_defaults);
 
-- 
2.6.2

[PATCH v3 4/5] tty: Move serial8250_stop_rx in front of serial8250_start_tx

[Matwey V. Kornilov]

Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
 drivers/tty/serial/8250/8250_port.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 067ef55..3f6d8a2 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1286,6 +1286,19 @@ static void autoconfig_irq(struct uart_8250_port *up)
 	port->irq = (irq > 0) ? irq : 0;
 }
 
+static void serial8250_stop_rx(struct uart_port *port)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+
+	serial8250_rpm_get(up);
+
+	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
+	up->port.read_status_mask &= ~UART_LSR_DR;
+	serial_port_out(port, UART_IER, up->ier);
+
+	serial8250_rpm_put(up);
+}
+
 static inline void __stop_tx(struct uart_8250_port *p)
 {
 	if (p->ier & UART_IER_THRI) {
@@ -1353,19 +1366,6 @@ static void serial8250_unthrottle(struct uart_port *port)
 	port->unthrottle(port);
 }
 
-static void serial8250_stop_rx(struct uart_port *port)
-{
-	struct uart_8250_port *up = up_to_u8250p(port);
-
-	serial8250_rpm_get(up);
-
-	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
-	up->port.read_status_mask &= ~UART_LSR_DR;
-	serial_port_out(port, UART_IER, up->ier);
-
-	serial8250_rpm_put(up);
-}
-
 static void serial8250_disable_ms(struct uart_port *port)
 {
 	struct uart_8250_port *up =
-- 
2.6.2

[PATCH v3 5/5] tty: Add software emulated RS485 support for 8250

[Matwey V. Kornilov]

Implementation of software emulation of RS485 direction handling is based
on omap-serial driver.
Before and after transmission RTS is set to the appropriate value.

Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
 drivers/tty/serial/8250/8250_port.c | 152 ++++++++++++++++++++++++++++++++++--
 include/linux/serial_8250.h         |   2 +
 2 files changed, 148 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3f6d8a2..b863a12 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -37,6 +37,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/pm_runtime.h>
+#include <linux/timer.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -1299,7 +1300,75 @@ static void serial8250_stop_rx(struct uart_port *port)
 	serial8250_rpm_put(up);
 }
 
-static inline void __stop_tx(struct uart_8250_port *p)
+static inline void serial8250_rs485_set_rts_on_send(struct uart_8250_port *p)
+{
+	if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
+		p->mcr |= UART_MCR_RTS;
+	else
+		p->mcr &= ~UART_MCR_RTS;
+	serial_out(p, UART_MCR, p->mcr);
+}
+
+static inline void serial8250_rs485_set_rts_after_send(struct uart_8250_port *p)
+{
+	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
+		p->mcr |= UART_MCR_RTS;
+	else
+		p->mcr &= ~UART_MCR_RTS;
+	serial_out(p, UART_MCR, p->mcr);
+}
+
+static __u32 serial8250_rs485_start_tx(struct uart_8250_port *p)
+{
+	if (p->capabilities & UART_CAP_HW485 || !(p->port.rs485.flags & SER_RS485_ENABLED))
+		return 0;
+	
+	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
+		serial8250_stop_rx(&p->port);
+	
+	del_timer_sync(&p->rs485_stop_tx_timer);
+
+	if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(p->mcr & UART_MCR_RTS)) {
+		serial8250_rs485_set_rts_on_send(p);
+		return p->port.rs485.delay_rts_before_send;
+	}
+
+	return 0;
+}
+
+static inline void __do_stop_tx_rs485(struct uart_8250_port *p)
+{
+	serial8250_rs485_set_rts_after_send(p);
+	/*
+	* Empty the RX FIFO, we are not interested in anything
+	* received during the half-duplex transmission.
+	*/
+	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
+		serial8250_clear_fifos(p);
+}
+
+static void serial8250_handle_rs485_stop_tx_timer(unsigned long arg)
+{
+	struct uart_8250_port *p = (struct uart_8250_port*)arg;
+	__do_stop_tx_rs485(p);
+}
+
+static inline void __stop_tx_rs485(struct uart_8250_port *p)
+{
+	if (p->capabilities & UART_CAP_HW485 || !(p->port.rs485.flags & SER_RS485_ENABLED))
+		return;
+
+	del_timer_sync(&p->rs485_start_tx_timer);
+
+	/* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
+	if (p->port.rs485.delay_rts_after_send > 0) {
+		mod_timer(&p->rs485_stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
+	} else {
+		__do_stop_tx_rs485(p);
+	}
+}
+
+static inline void __do_stop_tx(struct uart_8250_port *p)
 {
 	if (p->ier & UART_IER_THRI) {
 		p->ier &= ~UART_IER_THRI;
@@ -1308,6 +1377,12 @@ static inline void __stop_tx(struct uart_8250_port *p)
 	}
 }
 
+static inline void __stop_tx(struct uart_8250_port *p)
+{
+	__do_stop_tx(p);
+	__stop_tx_rs485(p);
+}
+
 static void serial8250_stop_tx(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
@@ -1325,12 +1400,10 @@ static void serial8250_stop_tx(struct uart_port *port)
 	serial8250_rpm_put(up);
 }
 
-static void serial8250_start_tx(struct uart_port *port)
+static inline void __start_tx(struct uart_port* port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 
-	serial8250_rpm_get_tx(up);
-
 	if (up->dma && !up->dma->tx_dma(up))
 		return;
 
@@ -1356,6 +1429,29 @@ static void serial8250_start_tx(struct uart_port *port)
 	}
 }
 
+static void serial8250_handle_rs485_start_tx_timer(unsigned long arg)
+{
+	struct uart_port* port = (struct uart_port*)arg;
+	__start_tx(port);
+}
+
+static void serial8250_start_tx(struct uart_port *port)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	__u32 delay;
+	
+	serial8250_rpm_get_tx(up);
+
+	if (timer_pending(&up->rs485_start_tx_timer))
+		return;
+
+	if ((delay = serial8250_rs485_start_tx(up))) {
+		mod_timer(&up->rs485_start_tx_timer, jiffies + delay * HZ / 1000);
+	} else {
+		__start_tx(port);
+	}
+}
+
 static void serial8250_throttle(struct uart_port *port)
 {
 	port->throttle(port);
@@ -1806,6 +1902,16 @@ static void serial8250_put_poll_char(struct uart_port *port,
 
 #endif /* CONFIG_CONSOLE_POLL */
 
+static inline void serial8250_startup_rs485(struct uart_port *port)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	
+	if (up->capabilities & UART_CAP_HW485 || !(up->port.rs485.flags & SER_RS485_ENABLED))
+		return;
+
+	serial8250_rs485_set_rts_after_send(up);
+}
+
 int serial8250_do_startup(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
@@ -2044,6 +2150,8 @@ dont_test_tx_en:
 		inb_p(icp);
 	}
 	retval = 0;
+
+	serial8250_startup_rs485(port);
 out:
 	serial8250_rpm_put(up);
 	return retval;
@@ -2057,12 +2165,24 @@ static int serial8250_startup(struct uart_port *port)
 	return serial8250_do_startup(port);
 }
 
+static inline void serial8250_shutdown_rs485(struct uart_port *port)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	
+	if (up->capabilities & UART_CAP_HW485 || !(up->port.rs485.flags & SER_RS485_ENABLED))
+		return;
+
+	del_timer_sync(&up->rs485_start_tx_timer);
+	del_timer_sync(&up->rs485_stop_tx_timer);
+}
+
 void serial8250_do_shutdown(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 	unsigned long flags;
 
 	serial8250_rpm_get(up);
+	serial8250_shutdown_rs485(port);
 	/*
 	 * Disable interrupts from this port
 	 */
@@ -2735,11 +2855,23 @@ serial8250_type(struct uart_port *port)
 	return uart_config[type].name;
 }
 
-static int serial8250_rs485_config(struct uart_port *port,
-				struct serial_rs485 *rs485)
+static int serial8250_rs485_config(struct uart_port *port, struct serial_rs485 *rs485)
 {
+	bool need_startup = false;
+
+	if (!(port->rs485.flags & SER_RS485_ENABLED) && (rs485->flags & SER_RS485_ENABLED)) {
+		need_startup = true;
+	}
+	if ((port->rs485.flags & SER_RS485_ENABLED) && !(rs485->flags & SER_RS485_ENABLED)) {
+		serial8250_shutdown_rs485(port);
+	}
+
 	port->rs485 = *rs485;
 	port->rs485.flags |= SER_RS485_SOFTWARE;
+
+	if (need_startup)
+		serial8250_startup_rs485(port);
+
 	return 0;
 }
 
@@ -2807,6 +2939,14 @@ void serial8250_set_defaults(struct uart_8250_port *up)
 	}
 
 	up->port.rs485_config = serial8250_rs485_config;
+	init_timer(&up->rs485_stop_tx_timer);
+	up->rs485_stop_tx_timer.function = serial8250_handle_rs485_stop_tx_timer;
+	up->rs485_stop_tx_timer.data = (unsigned long)up;
+	up->rs485_stop_tx_timer.flags |= TIMER_IRQSAFE;
+	init_timer(&up->rs485_start_tx_timer);
+	up->rs485_start_tx_timer.function = serial8250_handle_rs485_start_tx_timer;
+	up->rs485_start_tx_timer.data = (unsigned long)up;
+	up->rs485_start_tx_timer.flags |= TIMER_IRQSAFE;
 }
 EXPORT_SYMBOL_GPL(serial8250_set_defaults);
 
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index faa0e03..c4905e7 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -86,6 +86,8 @@ struct uart_8250_ops {
 struct uart_8250_port {
 	struct uart_port	port;
 	struct timer_list	timer;		/* "no irq" timer */
+	struct timer_list	rs485_start_tx_timer; /* "rs485 start tx" timer */
+	struct timer_list	rs485_stop_tx_timer; /* "rs485 stop tx" timer */
 	struct list_head	list;		/* ports on this IRQ */
 	unsigned short		capabilities;	/* port capabilities */
 	unsigned short		bugs;		/* port bugs */
-- 
2.6.2

Re: [PATCH v3 5/5] tty: Add software emulated RS485 support for 8250

[Andy Shevchenko]

On Thu, Nov 12, 2015 at 4:33 PM, Matwey V. Kornilov <matwey@sai.msu.ru> wrote:
> Implementation of software emulation of RS485 direction handling is based
> on omap-serial driver.
> Before and after transmission RTS is set to the appropriate value.
>

One comment below.

> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> ---
>  drivers/tty/serial/8250/8250_port.c | 152 ++++++++++++++++++++++++++++++++++--
>  include/linux/serial_8250.h         |   2 +
>  2 files changed, 148 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 3f6d8a2..b863a12 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -37,6 +37,7 @@
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/timer.h>
>
>  #include <asm/io.h>
>  #include <asm/irq.h>
> @@ -1299,7 +1300,75 @@ static void serial8250_stop_rx(struct uart_port *port)
>         serial8250_rpm_put(up);
>  }
>
> -static inline void __stop_tx(struct uart_8250_port *p)
> +static inline void serial8250_rs485_set_rts_on_send(struct uart_8250_port *p)
> +{
> +       if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
> +               p->mcr |= UART_MCR_RTS;
> +       else
> +               p->mcr &= ~UART_MCR_RTS;
> +       serial_out(p, UART_MCR, p->mcr);
> +}
> +
> +static inline void serial8250_rs485_set_rts_after_send(struct uart_8250_port *p)
> +{
> +       if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
> +               p->mcr |= UART_MCR_RTS;
> +       else
> +               p->mcr &= ~UART_MCR_RTS;
> +       serial_out(p, UART_MCR, p->mcr);
> +}
> +
> +static __u32 serial8250_rs485_start_tx(struct uart_8250_port *p)
> +{
> +       if (p->capabilities & UART_CAP_HW485 || !(p->port.rs485.flags & SER_RS485_ENABLED))
> +               return 0;
> +
> +       if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
> +               serial8250_stop_rx(&p->port);
> +
> +       del_timer_sync(&p->rs485_stop_tx_timer);
> +
> +       if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(p->mcr & UART_MCR_RTS)) {
> +               serial8250_rs485_set_rts_on_send(p);
> +               return p->port.rs485.delay_rts_before_send;
> +       }
> +
> +       return 0;
> +}
> +
> +static inline void __do_stop_tx_rs485(struct uart_8250_port *p)
> +{
> +       serial8250_rs485_set_rts_after_send(p);
> +       /*
> +       * Empty the RX FIFO, we are not interested in anything
> +       * received during the half-duplex transmission.
> +       */
> +       if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
> +               serial8250_clear_fifos(p);
> +}
> +
> +static void serial8250_handle_rs485_stop_tx_timer(unsigned long arg)
> +{
> +       struct uart_8250_port *p = (struct uart_8250_port*)arg;
> +       __do_stop_tx_rs485(p);
> +}
> +
> +static inline void __stop_tx_rs485(struct uart_8250_port *p)
> +{
> +       if (p->capabilities & UART_CAP_HW485 || !(p->port.rs485.flags & SER_RS485_ENABLED))
> +               return;

It seems you are using this pattern three times. What about

static inline bool is_rs485_sw_enabled(struct uart_8250_port *p)
{
      return !(p->capabilities & UART_CAP_HW485) &&
(p->port.rs485.flags & SER_RS485_ENABLED);
}

> +
> +       del_timer_sync(&p->rs485_start_tx_timer);
> +
> +       /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
> +       if (p->port.rs485.delay_rts_after_send > 0) {
> +               mod_timer(&p->rs485_stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
> +       } else {
> +               __do_stop_tx_rs485(p);
> +       }
> +}
> +
> +static inline void __do_stop_tx(struct uart_8250_port *p)
>  {
>         if (p->ier & UART_IER_THRI) {
>                 p->ier &= ~UART_IER_THRI;
> @@ -1308,6 +1377,12 @@ static inline void __stop_tx(struct uart_8250_port *p)
>         }
>  }
>
> +static inline void __stop_tx(struct uart_8250_port *p)
> +{
> +       __do_stop_tx(p);
> +       __stop_tx_rs485(p);
> +}
> +
>  static void serial8250_stop_tx(struct uart_port *port)
>  {
>         struct uart_8250_port *up = up_to_u8250p(port);
> @@ -1325,12 +1400,10 @@ static void serial8250_stop_tx(struct uart_port *port)
>         serial8250_rpm_put(up);
>  }
>
> -static void serial8250_start_tx(struct uart_port *port)
> +static inline void __start_tx(struct uart_port* port)
>  {
>         struct uart_8250_port *up = up_to_u8250p(port);
>
> -       serial8250_rpm_get_tx(up);
> -
>         if (up->dma && !up->dma->tx_dma(up))
>                 return;
>
> @@ -1356,6 +1429,29 @@ static void serial8250_start_tx(struct uart_port *port)
>         }
>  }
>
> +static void serial8250_handle_rs485_start_tx_timer(unsigned long arg)
> +{
> +       struct uart_port* port = (struct uart_port*)arg;
> +       __start_tx(port);
> +}
> +
> +static void serial8250_start_tx(struct uart_port *port)
> +{
> +       struct uart_8250_port *up = up_to_u8250p(port);
> +       __u32 delay;
> +
> +       serial8250_rpm_get_tx(up);
> +
> +       if (timer_pending(&up->rs485_start_tx_timer))
> +               return;
> +
> +       if ((delay = serial8250_rs485_start_tx(up))) {
> +               mod_timer(&up->rs485_start_tx_timer, jiffies + delay * HZ / 1000);
> +       } else {
> +               __start_tx(port);
> +       }
> +}
> +
>  static void serial8250_throttle(struct uart_port *port)
>  {
>         port->throttle(port);
> @@ -1806,6 +1902,16 @@ static void serial8250_put_poll_char(struct uart_port *port,
>
>  #endif /* CONFIG_CONSOLE_POLL */
>
> +static inline void serial8250_startup_rs485(struct uart_port *port)
> +{
> +       struct uart_8250_port *up = up_to_u8250p(port);
> +
> +       if (up->capabilities & UART_CAP_HW485 || !(up->port.rs485.flags & SER_RS485_ENABLED))
> +               return;
> +
> +       serial8250_rs485_set_rts_after_send(up);
> +}
> +
>  int serial8250_do_startup(struct uart_port *port)
>  {
>         struct uart_8250_port *up = up_to_u8250p(port);
> @@ -2044,6 +2150,8 @@ dont_test_tx_en:
>                 inb_p(icp);
>         }
>         retval = 0;
> +
> +       serial8250_startup_rs485(port);
>  out:
>         serial8250_rpm_put(up);
>         return retval;
> @@ -2057,12 +2165,24 @@ static int serial8250_startup(struct uart_port *port)
>         return serial8250_do_startup(port);
>  }
>
> +static inline void serial8250_shutdown_rs485(struct uart_port *port)
> +{
> +       struct uart_8250_port *up = up_to_u8250p(port);
> +
> +       if (up->capabilities & UART_CAP_HW485 || !(up->port.rs485.flags & SER_RS485_ENABLED))
> +               return;
> +
> +       del_timer_sync(&up->rs485_start_tx_timer);
> +       del_timer_sync(&up->rs485_stop_tx_timer);
> +}
> +
>  void serial8250_do_shutdown(struct uart_port *port)
>  {
>         struct uart_8250_port *up = up_to_u8250p(port);
>         unsigned long flags;
>
>         serial8250_rpm_get(up);
> +       serial8250_shutdown_rs485(port);
>         /*
>          * Disable interrupts from this port
>          */
> @@ -2735,11 +2855,23 @@ serial8250_type(struct uart_port *port)
>         return uart_config[type].name;
>  }
>
> -static int serial8250_rs485_config(struct uart_port *port,
> -                               struct serial_rs485 *rs485)
> +static int serial8250_rs485_config(struct uart_port *port, struct serial_rs485 *rs485)
>  {
> +       bool need_startup = false;
> +
> +       if (!(port->rs485.flags & SER_RS485_ENABLED) && (rs485->flags & SER_RS485_ENABLED)) {
> +               need_startup = true;
> +       }
> +       if ((port->rs485.flags & SER_RS485_ENABLED) && !(rs485->flags & SER_RS485_ENABLED)) {
> +               serial8250_shutdown_rs485(port);
> +       }
> +
>         port->rs485 = *rs485;
>         port->rs485.flags |= SER_RS485_SOFTWARE;
> +
> +       if (need_startup)
> +               serial8250_startup_rs485(port);
> +
>         return 0;
>  }
>
> @@ -2807,6 +2939,14 @@ void serial8250_set_defaults(struct uart_8250_port *up)
>         }
>
>         up->port.rs485_config = serial8250_rs485_config;
> +       init_timer(&up->rs485_stop_tx_timer);
> +       up->rs485_stop_tx_timer.function = serial8250_handle_rs485_stop_tx_timer;
> +       up->rs485_stop_tx_timer.data = (unsigned long)up;
> +       up->rs485_stop_tx_timer.flags |= TIMER_IRQSAFE;
> +       init_timer(&up->rs485_start_tx_timer);
> +       up->rs485_start_tx_timer.function = serial8250_handle_rs485_start_tx_timer;
> +       up->rs485_start_tx_timer.data = (unsigned long)up;
> +       up->rs485_start_tx_timer.flags |= TIMER_IRQSAFE;
>  }
>  EXPORT_SYMBOL_GPL(serial8250_set_defaults);
>
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index faa0e03..c4905e7 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -86,6 +86,8 @@ struct uart_8250_ops {
>  struct uart_8250_port {
>         struct uart_port        port;
>         struct timer_list       timer;          /* "no irq" timer */
> +       struct timer_list       rs485_start_tx_timer; /* "rs485 start tx" timer */
> +       struct timer_list       rs485_stop_tx_timer; /* "rs485 stop tx" timer */
>         struct list_head        list;           /* ports on this IRQ */
>         unsigned short          capabilities;   /* port capabilities */
>         unsigned short          bugs;           /* port bugs */
> --
> 2.6.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[One Thousand Gnomes]

On Thu, 12 Nov 2015 17:33:53 +0300
"Matwey V. Kornilov" <matwey@sai.msu.ru> wrote:

> This flag is supposed to be used by uart drivers using software rs485 direction control.
> 
> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> ---
>  include/uapi/linux/serial.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> index 25331f9..95b15ca 100644
> --- a/include/uapi/linux/serial.h
> +++ b/include/uapi/linux/serial.h
> @@ -121,6 +121,9 @@ struct serial_rs485 {
>  #define SER_RS485_RTS_AFTER_SEND	(1 << 2)	/* Logical level for
>  							   RTS pin after sent*/
>  #define SER_RS485_RX_DURING_TX		(1 << 4)
> +#define SER_RS485_SOFTWARE		(1 << 5)	/* Software
> +							   implementation is
> +							   being used */

I've only got one question here - why do we need this flag. Why does the
application care whether the timer is in the kernel or in the chip. In
particular think about cases where some combinations of features require
software fallback and others don't. What would the flag indicate then.

The patches look nice but I'd strongly favour not having a software flag.
It should never matter as the kernel API is the same in all cases and we
should therefore discourage application code from trying to know things
it doesn't need to worry about.

Alan

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Peter Hurley]

On 11/12/2015 02:57 PM, One Thousand Gnomes wrote:
> On Thu, 12 Nov 2015 17:33:53 +0300
> "Matwey V. Kornilov" <matwey@sai.msu.ru> wrote:
> 
>> This flag is supposed to be used by uart drivers using software rs485 direction control.
>>
>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>> ---
>>  include/uapi/linux/serial.h | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
>> index 25331f9..95b15ca 100644
>> --- a/include/uapi/linux/serial.h
>> +++ b/include/uapi/linux/serial.h
>> @@ -121,6 +121,9 @@ struct serial_rs485 {
>>  #define SER_RS485_RTS_AFTER_SEND	(1 << 2)	/* Logical level for
>>  							   RTS pin after sent*/
>>  #define SER_RS485_RX_DURING_TX		(1 << 4)
>> +#define SER_RS485_SOFTWARE		(1 << 5)	/* Software
>> +							   implementation is
>> +							   being used */
> 
> I've only got one question here - why do we need this flag. Why does the
> application care whether the timer is in the kernel or in the chip. In
> particular think about cases where some combinations of features require
> software fallback and others don't. What would the flag indicate then.
> 
> The patches look nice but I'd strongly favour not having a software flag.
> It should never matter as the kernel API is the same in all cases and we
> should therefore discourage application code from trying to know things
> it doesn't need to worry about.

I specifically asked for it.

I can think of 2 reasons that userspace wants to know:
1. Because the characteristics of the software emulation are unacceptable so
   the application wants to terminate w/error rather than continue.
2. Because userspace will use different values for h/w vs. s/w. For example,
   right now, the emulation will raise/lower RTS prematurely when tx ends if
   the rts-after-send timer is 0.

I agree that combination features might be problematic.
An illustrative (kernel-space) example is the mess that is dmaengine_pause().
Some DMA implementations provide the means to stop and restart DMA without
losing data and some DMA implementations do not. Unfortunately, some
advertise they support dmaengine_pause() but only for lossy uses like audio.
Because the api hides this, the query interface for pause support is
useless.

Regards,
Peter Hurley

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Andy Shevchenko]

On Thu, Nov 12, 2015 at 10:22 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 11/12/2015 02:57 PM, One Thousand Gnomes wrote:

> An illustrative (kernel-space) example is the mess that is dmaengine_pause().
> Some DMA implementations provide the means to stop and restart DMA without
> losing data and some DMA implementations do not. Unfortunately, some
> advertise they support dmaengine_pause() but only for lossy uses like audio.
> Because the api hides this, the query interface for pause support is
> useless.

The DMA pause() call means only pause with possibility to resume.
There is a resume() call as well. Any driver which treats pause() as a
complete stop is buggy driver and should be fixed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Peter Hurley]

On 11/12/2015 07:41 PM, Andy Shevchenko wrote:
> On Thu, Nov 12, 2015 at 10:22 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 11/12/2015 02:57 PM, One Thousand Gnomes wrote:
> 
>> An illustrative (kernel-space) example is the mess that is dmaengine_pause().
>> Some DMA implementations provide the means to stop and restart DMA without
>> losing data and some DMA implementations do not. Unfortunately, some
>> advertise they support dmaengine_pause() but only for lossy uses like audio.
>> Because the api hides this, the query interface for pause support is
>> useless.
> 
> The DMA pause() call means only pause with possibility to resume.
> There is a resume() call as well. Any driver which treats pause() as a
> complete stop is buggy driver and should be fixed.

How about pause _without_ the possibility to resume?

https://groups.google.com/d/msg/linux.kernel/Abe0hfGcgsw/H0se55wC558J

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Andy Shevchenko]

On Fri, Nov 13, 2015 at 3:11 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 11/12/2015 07:41 PM, Andy Shevchenko wrote:
>> On Thu, Nov 12, 2015 at 10:22 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> On 11/12/2015 02:57 PM, One Thousand Gnomes wrote:
>>
>>> An illustrative (kernel-space) example is the mess that is dmaengine_pause().
>>> Some DMA implementations provide the means to stop and restart DMA without
>>> losing data and some DMA implementations do not. Unfortunately, some
>>> advertise they support dmaengine_pause() but only for lossy uses like audio.
>>> Because the api hides this, the query interface for pause support is
>>> useless.
>>
>> The DMA pause() call means only pause with possibility to resume.
>> There is a resume() call as well. Any driver which treats pause() as a
>> complete stop is buggy driver and should be fixed.
>
> How about pause _without_ the possibility to resume?
>
> https://groups.google.com/d/msg/linux.kernel/Abe0hfGcgsw/H0se55wC558J

Briefly what I got from the thread that Russel shows similar view on
the API, so that's why he was objecting to add pause/resume calls for
a specific hardware.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Peter Hurley]

On 11/12/2015 08:26 PM, Andy Shevchenko wrote:
> On Fri, Nov 13, 2015 at 3:11 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 11/12/2015 07:41 PM, Andy Shevchenko wrote:
>>> On Thu, Nov 12, 2015 at 10:22 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>>> On 11/12/2015 02:57 PM, One Thousand Gnomes wrote:
>>>
>>>> An illustrative (kernel-space) example is the mess that is dmaengine_pause().
>>>> Some DMA implementations provide the means to stop and restart DMA without
>>>> losing data and some DMA implementations do not. Unfortunately, some
>>>> advertise they support dmaengine_pause() but only for lossy uses like audio.
>>>> Because the api hides this, the query interface for pause support is
>>>> useless.
>>>
>>> The DMA pause() call means only pause with possibility to resume.
>>> There is a resume() call as well. Any driver which treats pause() as a
>>> complete stop is buggy driver and should be fixed.
>>
>> How about pause _without_ the possibility to resume?
>>
>> https://groups.google.com/d/msg/linux.kernel/Abe0hfGcgsw/H0se55wC558J
> 
> Briefly what I got from the thread that Russell shows similar view on
> the API, so that's why he was objecting to add pause/resume calls for
> a specific hardware.

Not quite.

That dmaengine driver (omap-dma) advertises that it supports pause() via
dma_get_slave_caps(). And if you call it with a cyclic channel it will pause.
However, if you call dmaengine_pause() with a slave channel it returns an error
*because the hardware can't actually meet the criteria for dmaengine_pause()*
which is pause()/resume() without data loss.

IOW, there is no method of determining a priori if dmaengine_pause() will
categorically fail for a given transfer type.

Regards,
Peter Hurley

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Matwey V. Kornilov]

2015-11-12 22:57 GMT+03:00 One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>:
> On Thu, 12 Nov 2015 17:33:53 +0300
> "Matwey V. Kornilov" <matwey@sai.msu.ru> wrote:
>
>> This flag is supposed to be used by uart drivers using software rs485 direction control.
>>
>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>> ---
>>  include/uapi/linux/serial.h | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
>> index 25331f9..95b15ca 100644
>> --- a/include/uapi/linux/serial.h
>> +++ b/include/uapi/linux/serial.h
>> @@ -121,6 +121,9 @@ struct serial_rs485 {
>>  #define SER_RS485_RTS_AFTER_SEND     (1 << 2)        /* Logical level for
>>                                                          RTS pin after sent*/
>>  #define SER_RS485_RX_DURING_TX               (1 << 4)
>> +#define SER_RS485_SOFTWARE           (1 << 5)        /* Software
>> +                                                        implementation is
>> +                                                        being used */
>
> I've only got one question here - why do we need this flag. Why does the
> application care whether the timer is in the kernel or in the chip. In
> particular think about cases where some combinations of features require
> software fallback and others don't. What would the flag indicate then.
>

Peter asked for it, I respect his experience.
Only two lines are required to implement this, so it is easy to add,
easy to drop.

> The patches look nice but I'd strongly favour not having a software flag.
> It should never matter as the kernel API is the same in all cases and we
> should therefore discourage application code from trying to know things
> it doesn't need to worry about.
>
> Alan
>



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

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[One Thousand Gnomes]

> I specifically asked for it.
> 
> I can think of 2 reasons that userspace wants to know:
> 1. Because the characteristics of the software emulation are unacceptable so
>    the application wants to terminate w/error rather than continue.

But that could equally be true of hardware. In fact your software
emulation is going to behave vastly better than many of the hardware ones.

> 2. Because userspace will use different values for h/w vs. s/w. For example,
>    right now, the emulation will raise/lower RTS prematurely when tx ends if
>    the rts-after-send timer is 0.

That's a bug then. It should be fixed as part of the merge or future
patches - if they are not providing that emulation then they ought to do
so and at least adjust the timing based on the baud rate so you don't
have to spin polling the 16x50 uart to check the last bit fell out of the
register.

I'd have no problem with an API that was about asking what features are
available : both hardware and software - but the software flag seems to
make no sense at all. Software doesn't imply anything about quality or
feature set. If there is something the emulation cannot support then
there should be a flag indicating that feature is not supported, not a
flag saying software (which means nothing - as it may be supported in
future, or may differ by uart etc).

It's also not "easy to drop". If it ever goes in we are stuck with a
pointless impossible to correctly set flag for all eternity.

Please explain the correct setting for this flag when a device driver
uses hardware or software or a mix according to what the silicon is
capable of and what values are requested ? How will an application use the
flag meaningfully. Please explain what will happen if someone discovers a
silicon bug and in a future 4.x release turns an implementation from
hardware to software - will they have to lie about the flag to avoid
breaking their application code - that strikes me as a bad thing.

At the very least the above should be clearly explained in the
documentation and patch covering notes - and if nobody can explain those
then IMHO the flag is broken.

Alan

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Peter Hurley]

On 11/14/2015 10:25 AM, One Thousand Gnomes wrote:
>> I specifically asked for it.
>>
>> I can think of 2 reasons that userspace wants to know:
>> 1. Because the characteristics of the software emulation are unacceptable so
>>    the application wants to terminate w/error rather than continue.
> 
> But that could equally be true of hardware.

I had this exact same thought, but concluded that it argues for a way
to select the software implementation even when h/w supports RS485.

> In fact your software
> emulation is going to behave vastly better than many of the hardware ones.
> 
>> 2. Because userspace will use different values for h/w vs. s/w. For example,
>>    right now, the emulation will raise/lower RTS prematurely when tx ends if
>>    the rts-after-send timer is 0.
> 
> That's a bug then. It should be fixed as part of the merge or future
> patches - if they are not providing that emulation then they ought to do
> so and at least adjust the timing based on the baud rate so you don't
> have to spin polling the 16x50 uart to check the last bit fell out of the
> register.

I suppose the timer(s) could be fudged and then TEMT polled (or polled every
char baud cycles). But I don't see how this will behave better than a h/w
implementation; the granularity of the jiffy clock alone will guarantee
sub-optimal turnaround, even at 9600.

> I'd have no problem with an API that was about asking what features are
> available : both hardware and software - but the software flag seems to
> make no sense at all. Software doesn't imply anything about quality or
> feature set. If there is something the emulation cannot support then
> there should be a flag indicating that feature is not supported, not a
> flag saying software (which means nothing - as it may be supported in
> future, or may differ by uart etc).

Fair enough.

> It's also not "easy to drop". If it ever goes in we are stuck with a
> pointless impossible to correctly set flag for all eternity.
> 
> Please explain the correct setting for this flag when a device driver
> uses hardware or software or a mix according to what the silicon is
> capable of and what values are requested ? How will an application use the
> flag meaningfully. Please explain what will happen if someone discovers a
> silicon bug and in a future 4.x release turns an implementation from
> hardware to software - will they have to lie about the flag to avoid
> breaking their application code - that strikes me as a bad thing.

The existing driver behavior is already significantly variant and needs
to be converged, which shouldn't be too difficult. Here's a quick summary:

mcfuart		ignores delay values, delays unsupported
imx		clamps delay values to 0, delays unsupported
atmel		only delay_rts_after_send used; delay_rts_before_send does nothing
8250_fintek	clamps delay values to 1, unclear if h/w delay is msecs
omap-serial*	software emulation (but tx empty polling not reqd)
lpc18xx-uart	clamps delay_rts_before_send to 0, unsupported
		clamps delay_rts_after_send to max h/w value
max310x		returns -ERANGE if either delay value > h/w support (15 msecs)
sc16is7xx*	returns -EINVAL if delay_rts_after_send is set
crisv10*	clamps delay_rts_before_send to 1000 msecs
		ignores delays_rts_after_send (after dma is delayed by 2 * chars)
* implements delay(s) in software

The omap-serial emulation should not have been merged in its current form.

IMO the proper driver behavior should be clamp to h/w limit so an application
can determine the maximum delay supported. If a delay is unsupported, it should
be clamped to 0. The application should check the RS485 settings returned by
TIOCSRS485 to determine how the driver set them.
[ Documentation/serial/serial-rs485.txt should suggest/model this action ]

Are TIOCGRS485 and TIOCSRS485 documented in tty_ioctl man page? (I haven't
updated my man pages in a while)

As far as software vs. hardware and a query api, what I care about is
conveying to userspace whether the implementation will be adequate for purpose,
with the main issue being the true delay from actual EOT to RTS toggle
when delay_after_rts_send == 0.

Since that delay is unbounded with software methods, I thought it made sense to
indicate that with a read-only bit. Naming it something else is fine too;
SER_RS485_NOT_REALTIME_EOT?

A more comprehensive approach might be to add a capabilities word
to struct serial_rs485 which would allow the driver to report what
it supports; eg. realtime turnaround or not, etc. (Not sure if extending
struct serial_rs485 is really possible; the serial core hasn't been
clearing padding on the driver's behalf).

> At the very least the above should be clearly explained in the
> documentation and patch covering notes - and if nobody can explain those
> then IMHO the flag is broken.

Yep.

Regards,
Peter Hurley

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Matwey V. Kornilov]

2015-11-16 22:18 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> On 11/14/2015 10:25 AM, One Thousand Gnomes wrote:
>>> I specifically asked for it.
>>>
>>> I can think of 2 reasons that userspace wants to know:
>>> 1. Because the characteristics of the software emulation are unacceptable so
>>>    the application wants to terminate w/error rather than continue.
>>
>> But that could equally be true of hardware.
>
> I had this exact same thought, but concluded that it argues for a way
> to select the software implementation even when h/w supports RS485.
>
>> In fact your software
>> emulation is going to behave vastly better than many of the hardware ones.
>>
>>> 2. Because userspace will use different values for h/w vs. s/w. For example,
>>>    right now, the emulation will raise/lower RTS prematurely when tx ends if
>>>    the rts-after-send timer is 0.
>>
>> That's a bug then. It should be fixed as part of the merge or future
>> patches - if they are not providing that emulation then they ought to do
>> so and at least adjust the timing based on the baud rate so you don't
>> have to spin polling the 16x50 uart to check the last bit fell out of the
>> register.
>
> I suppose the timer(s) could be fudged and then TEMT polled (or polled every
> char baud cycles). But I don't see how this will behave better than a h/w
> implementation; the granularity of the jiffy clock alone will guarantee
> sub-optimal turnaround, even at 9600.
>
>> I'd have no problem with an API that was about asking what features are
>> available : both hardware and software - but the software flag seems to
>> make no sense at all. Software doesn't imply anything about quality or
>> feature set. If there is something the emulation cannot support then
>> there should be a flag indicating that feature is not supported, not a
>> flag saying software (which means nothing - as it may be supported in
>> future, or may differ by uart etc).
>
> Fair enough.
>
>> It's also not "easy to drop". If it ever goes in we are stuck with a
>> pointless impossible to correctly set flag for all eternity.
>>
>> Please explain the correct setting for this flag when a device driver
>> uses hardware or software or a mix according to what the silicon is
>> capable of and what values are requested ? How will an application use the
>> flag meaningfully. Please explain what will happen if someone discovers a
>> silicon bug and in a future 4.x release turns an implementation from
>> hardware to software - will they have to lie about the flag to avoid
>> breaking their application code - that strikes me as a bad thing.
>
> The existing driver behavior is already significantly variant and needs
> to be converged, which shouldn't be too difficult. Here's a quick summary:
>
> mcfuart         ignores delay values, delays unsupported
> imx             clamps delay values to 0, delays unsupported
> atmel           only delay_rts_after_send used; delay_rts_before_send does nothing
> 8250_fintek     clamps delay values to 1, unclear if h/w delay is msecs
> omap-serial*    software emulation (but tx empty polling not reqd)
> lpc18xx-uart    clamps delay_rts_before_send to 0, unsupported
>                 clamps delay_rts_after_send to max h/w value
> max310x         returns -ERANGE if either delay value > h/w support (15 msecs)
> sc16is7xx*      returns -EINVAL if delay_rts_after_send is set
> crisv10*        clamps delay_rts_before_send to 1000 msecs
>                 ignores delays_rts_after_send (after dma is delayed by 2 * chars)
> * implements delay(s) in software
>
> The omap-serial emulation should not have been merged in its current form.
>
> IMO the proper driver behavior should be clamp to h/w limit so an application
> can determine the maximum delay supported. If a delay is unsupported, it should
> be clamped to 0. The application should check the RS485 settings returned by
> TIOCSRS485 to determine how the driver set them.
> [ Documentation/serial/serial-rs485.txt should suggest/model this action ]

But the similar could be true for minimal supported delay.
If user requires delay which is less than lower bound, the delay is
raised to the lower bound.
If user requires delay which is greater than upper bound, the delay is
set to the upper bound.
Then software implementation could use (tx fifo size / baudrate) as
lower bound for delay_after_send.

>
> Are TIOCGRS485 and TIOCSRS485 documented in tty_ioctl man page? (I haven't
> updated my man pages in a while)
>
> As far as software vs. hardware and a query api, what I care about is
> conveying to userspace whether the implementation will be adequate for purpose,
> with the main issue being the true delay from actual EOT to RTS toggle
> when delay_after_rts_send == 0.

Or I just can internally add (tx fifo size / baudrate) to the user
supplied value to take care of the bytes in tx fifo.

>
> Since that delay is unbounded with software methods, I thought it made sense to
> indicate that with a read-only bit. Naming it something else is fine too;
> SER_RS485_NOT_REALTIME_EOT?
>
> A more comprehensive approach might be to add a capabilities word
> to struct serial_rs485 which would allow the driver to report what
> it supports; eg. realtime turnaround or not, etc. (Not sure if extending
> struct serial_rs485 is really possible; the serial core hasn't been
> clearing padding on the driver's behalf).
>
>> At the very least the above should be clearly explained in the
>> documentation and patch covering notes - and if nobody can explain those
>> then IMHO the flag is broken.
>
> Yep.
>
> Regards,
> Peter Hurley
>



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

Re: [PATCH v3 5/5] tty: Add software emulated RS485 support for 8250

[Uwe Kleine-König]

Hello,

On Thu, Nov 12, 2015 at 05:33:56PM +0300, Matwey V. Kornilov wrote:
> +static void serial8250_start_tx(struct uart_port *port)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	__u32 delay;
> +	
> +	serial8250_rpm_get_tx(up);
> +
> +	if (timer_pending(&up->rs485_start_tx_timer))
> +		return;

I think this is wrong (or at least suboptimal). The .start_tx callback
can be called even if transmission is already ongoing. In this case you
don't want to restart the rs485_start_tx_timer.

> +	if ((delay = serial8250_rs485_start_tx(up))) {
> +		mod_timer(&up->rs485_start_tx_timer, jiffies + delay * HZ / 1000);
> +	} else {
> +		__start_tx(port);
> +	}
> +}
> +
> [...]
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index faa0e03..c4905e7 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -86,6 +86,8 @@ struct uart_8250_ops {
>  struct uart_8250_port {
>  	struct uart_port	port;
>  	struct timer_list	timer;		/* "no irq" timer */
> +	struct timer_list	rs485_start_tx_timer; /* "rs485 start tx" timer */
> +	struct timer_list	rs485_stop_tx_timer; /* "rs485 stop tx" timer */

Do you really need two timers here? At each point in time there should
only be at most one of them active.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v3 5/5] tty: Add software emulated RS485 support for 8250

[Matwey V. Kornilov]

2015-11-17 12:24 GMT+03:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello,
>
> On Thu, Nov 12, 2015 at 05:33:56PM +0300, Matwey V. Kornilov wrote:
>> +static void serial8250_start_tx(struct uart_port *port)
>> +{
>> +     struct uart_8250_port *up = up_to_u8250p(port);
>> +     __u32 delay;
>> +
>> +     serial8250_rpm_get_tx(up);
>> +
>> +     if (timer_pending(&up->rs485_start_tx_timer))
>> +             return;
>
> I think this is wrong (or at least suboptimal). The .start_tx callback
> can be called even if transmission is already ongoing. In this case you
> don't want to restart the rs485_start_tx_timer.

If I understand you correctly, this maybe suboptimal but not wrong.
Calling .start_tx during transmission will lead to call
serial8250_rs485_start_tx which will return 0 because RTS is already
in proper state, and then __start_tx will be called immediately. Just
as it happens now.

>
>> +     if ((delay = serial8250_rs485_start_tx(up))) {
>> +             mod_timer(&up->rs485_start_tx_timer, jiffies + delay * HZ / 1000);
>> +     } else {
>> +             __start_tx(port);
>> +     }
>> +}
>> +
>> [...]
>> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
>> index faa0e03..c4905e7 100644
>> --- a/include/linux/serial_8250.h
>> +++ b/include/linux/serial_8250.h
>> @@ -86,6 +86,8 @@ struct uart_8250_ops {
>>  struct uart_8250_port {
>>       struct uart_port        port;
>>       struct timer_list       timer;          /* "no irq" timer */
>> +     struct timer_list       rs485_start_tx_timer; /* "rs485 start tx" timer */
>> +     struct timer_list       rs485_stop_tx_timer; /* "rs485 stop tx" timer */
>
> Do you really need two timers here? At each point in time there should
> only be at most one of them active.

You are right, only one is active. Having two timers is implicit way
to store the state ('starting' or 'stopping'). I don't think that this
is the worst way.

>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>



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

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Peter Hurley]

On 11/17/2015 03:20 AM, Matwey V. Kornilov wrote:
> 2015-11-16 22:18 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>> On 11/14/2015 10:25 AM, One Thousand Gnomes wrote:
>>>> I specifically asked for it.
>>>>
>>>> I can think of 2 reasons that userspace wants to know:
>>>> 1. Because the characteristics of the software emulation are unacceptable so
>>>>    the application wants to terminate w/error rather than continue.
>>>
>>> But that could equally be true of hardware.
>>
>> I had this exact same thought, but concluded that it argues for a way
>> to select the software implementation even when h/w supports RS485.
>>
>>> In fact your software
>>> emulation is going to behave vastly better than many of the hardware ones.
>>>
>>>> 2. Because userspace will use different values for h/w vs. s/w. For example,
>>>>    right now, the emulation will raise/lower RTS prematurely when tx ends if
>>>>    the rts-after-send timer is 0.
>>>
>>> That's a bug then. It should be fixed as part of the merge or future
>>> patches - if they are not providing that emulation then they ought to do
>>> so and at least adjust the timing based on the baud rate so you don't
>>> have to spin polling the 16x50 uart to check the last bit fell out of the
>>> register.
>>
>> I suppose the timer(s) could be fudged and then TEMT polled (or polled every
>> char baud cycles). But I don't see how this will behave better than a h/w
>> implementation; the granularity of the jiffy clock alone will guarantee
>> sub-optimal turnaround, even at 9600.
>>
>>> I'd have no problem with an API that was about asking what features are
>>> available : both hardware and software - but the software flag seems to
>>> make no sense at all. Software doesn't imply anything about quality or
>>> feature set. If there is something the emulation cannot support then
>>> there should be a flag indicating that feature is not supported, not a
>>> flag saying software (which means nothing - as it may be supported in
>>> future, or may differ by uart etc).
>>
>> Fair enough.
>>
>>> It's also not "easy to drop". If it ever goes in we are stuck with a
>>> pointless impossible to correctly set flag for all eternity.
>>>
>>> Please explain the correct setting for this flag when a device driver
>>> uses hardware or software or a mix according to what the silicon is
>>> capable of and what values are requested ? How will an application use the
>>> flag meaningfully. Please explain what will happen if someone discovers a
>>> silicon bug and in a future 4.x release turns an implementation from
>>> hardware to software - will they have to lie about the flag to avoid
>>> breaking their application code - that strikes me as a bad thing.
>>
>> The existing driver behavior is already significantly variant and needs
>> to be converged, which shouldn't be too difficult. Here's a quick summary:
>>
>> mcfuart         ignores delay values, delays unsupported
>> imx             clamps delay values to 0, delays unsupported
>> atmel           only delay_rts_after_send used; delay_rts_before_send does nothing
>> 8250_fintek     clamps delay values to 1, unclear if h/w delay is msecs
>> omap-serial*    software emulation (but tx empty polling not reqd)
>> lpc18xx-uart    clamps delay_rts_before_send to 0, unsupported
>>                 clamps delay_rts_after_send to max h/w value
>> max310x         returns -ERANGE if either delay value > h/w support (15 msecs)
>> sc16is7xx*      returns -EINVAL if delay_rts_after_send is set
>> crisv10*        clamps delay_rts_before_send to 1000 msecs
>>                 ignores delays_rts_after_send (after dma is delayed by 2 * chars)
>> * implements delay(s) in software
>>
>> The omap-serial emulation should not have been merged in its current form.
>>
>> IMO the proper driver behavior should be clamp to h/w limit so an application
>> can determine the maximum delay supported. If a delay is unsupported, it should
>> be clamped to 0. The application should check the RS485 settings returned by
>> TIOCSRS485 to determine how the driver set them.
>> [ Documentation/serial/serial-rs485.txt should suggest/model this action ]
> 
> But the similar could be true for minimal supported delay. If user
> requires delay which is less than lower bound, the delay is raised to
> the lower bound. If user requires delay which is greater than upper
> bound, the delay is set to the upper bound. Then software
> implementation could use (tx fifo size / baudrate) as lower bound for
> delay_after_send.

>From the application point-of-view (really the only relevant semantics),
delay_dts_after_send refers to the number of milliseconds to delay the
toggle of RTS after the last bit has been _transmitted_.

I agree with Alan that any adjustment to the delay to adhere to that
meaning needs to be transparent to user-space.


>> Are TIOCGRS485 and TIOCSRS485 documented in tty_ioctl man page? (I haven't
>> updated my man pages in a while)
>>
>> As far as software vs. hardware and a query api, what I care about is
>> conveying to userspace whether the implementation will be adequate for purpose,
>> with the main issue being the true delay from actual EOT to RTS toggle
>> when delay_after_rts_send == 0.
> 
> Or I just can internally add (tx fifo size / baudrate) to the user
> supplied value to take care of the bytes in tx fifo.

Yes. Or poll every jiffy.

But either will be far too coarse for many users; a delay_rts_after_send of
0 could still produce multi- _msec_ delays when the application expects
turnaround of ~1 char time. At a leisurely 19200 baud, that's ~520us which will
not be possible with this emulation.

A couple of possibilities for improving the emulation are:
1) Optionally using an HR timer for sub-jiffy turnaround.
2) Only supporting 8250-based hardware that can be set to interrupt when
   both tx fifo and transmitter shift register are empty.

But regardless, the driver should still advertise whether direction control
is realtime or not (ie., software or not).

Regards,
Peter Hurley


>> Since that delay is unbounded with software methods, I thought it made sense to
>> indicate that with a read-only bit. Naming it something else is fine too;
>> SER_RS485_NOT_REALTIME_EOT?
>>
>> A more comprehensive approach might be to add a capabilities word
>> to struct serial_rs485 which would allow the driver to report what
>> it supports; eg. realtime turnaround or not, etc. (Not sure if extending
>> struct serial_rs485 is really possible; the serial core hasn't been
>> clearing padding on the driver's behalf).
>>
>>> At the very least the above should be clearly explained in the
>>> documentation and patch covering notes - and if nobody can explain those
>>> then IMHO the flag is broken.
>>
>> Yep.
>>
>> Regards,
>> Peter Hurley

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Matwey V. Kornilov]

2015-11-18 21:33 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> On 11/17/2015 03:20 AM, Matwey V. Kornilov wrote:
>> 2015-11-16 22:18 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>> On 11/14/2015 10:25 AM, One Thousand Gnomes wrote:
>>>>> I specifically asked for it.
>>>>>
>>>>> I can think of 2 reasons that userspace wants to know:
>>>>> 1. Because the characteristics of the software emulation are unacceptable so
>>>>>    the application wants to terminate w/error rather than continue.
>>>>
>>>> But that could equally be true of hardware.
>>>
>>> I had this exact same thought, but concluded that it argues for a way
>>> to select the software implementation even when h/w supports RS485.
>>>
>>>> In fact your software
>>>> emulation is going to behave vastly better than many of the hardware ones.
>>>>
>>>>> 2. Because userspace will use different values for h/w vs. s/w. For example,
>>>>>    right now, the emulation will raise/lower RTS prematurely when tx ends if
>>>>>    the rts-after-send timer is 0.
>>>>
>>>> That's a bug then. It should be fixed as part of the merge or future
>>>> patches - if they are not providing that emulation then they ought to do
>>>> so and at least adjust the timing based on the baud rate so you don't
>>>> have to spin polling the 16x50 uart to check the last bit fell out of the
>>>> register.
>>>
>>> I suppose the timer(s) could be fudged and then TEMT polled (or polled every
>>> char baud cycles). But I don't see how this will behave better than a h/w
>>> implementation; the granularity of the jiffy clock alone will guarantee
>>> sub-optimal turnaround, even at 9600.
>>>
>>>> I'd have no problem with an API that was about asking what features are
>>>> available : both hardware and software - but the software flag seems to
>>>> make no sense at all. Software doesn't imply anything about quality or
>>>> feature set. If there is something the emulation cannot support then
>>>> there should be a flag indicating that feature is not supported, not a
>>>> flag saying software (which means nothing - as it may be supported in
>>>> future, or may differ by uart etc).
>>>
>>> Fair enough.
>>>
>>>> It's also not "easy to drop". If it ever goes in we are stuck with a
>>>> pointless impossible to correctly set flag for all eternity.
>>>>
>>>> Please explain the correct setting for this flag when a device driver
>>>> uses hardware or software or a mix according to what the silicon is
>>>> capable of and what values are requested ? How will an application use the
>>>> flag meaningfully. Please explain what will happen if someone discovers a
>>>> silicon bug and in a future 4.x release turns an implementation from
>>>> hardware to software - will they have to lie about the flag to avoid
>>>> breaking their application code - that strikes me as a bad thing.
>>>
>>> The existing driver behavior is already significantly variant and needs
>>> to be converged, which shouldn't be too difficult. Here's a quick summary:
>>>
>>> mcfuart         ignores delay values, delays unsupported
>>> imx             clamps delay values to 0, delays unsupported
>>> atmel           only delay_rts_after_send used; delay_rts_before_send does nothing
>>> 8250_fintek     clamps delay values to 1, unclear if h/w delay is msecs
>>> omap-serial*    software emulation (but tx empty polling not reqd)
>>> lpc18xx-uart    clamps delay_rts_before_send to 0, unsupported
>>>                 clamps delay_rts_after_send to max h/w value
>>> max310x         returns -ERANGE if either delay value > h/w support (15 msecs)
>>> sc16is7xx*      returns -EINVAL if delay_rts_after_send is set
>>> crisv10*        clamps delay_rts_before_send to 1000 msecs
>>>                 ignores delays_rts_after_send (after dma is delayed by 2 * chars)
>>> * implements delay(s) in software
>>>
>>> The omap-serial emulation should not have been merged in its current form.
>>>
>>> IMO the proper driver behavior should be clamp to h/w limit so an application
>>> can determine the maximum delay supported. If a delay is unsupported, it should
>>> be clamped to 0. The application should check the RS485 settings returned by
>>> TIOCSRS485 to determine how the driver set them.
>>> [ Documentation/serial/serial-rs485.txt should suggest/model this action ]
>>
>> But the similar could be true for minimal supported delay. If user
>> requires delay which is less than lower bound, the delay is raised to
>> the lower bound. If user requires delay which is greater than upper
>> bound, the delay is set to the upper bound. Then software
>> implementation could use (tx fifo size / baudrate) as lower bound for
>> delay_after_send.
>
> From the application point-of-view (really the only relevant semantics),
> delay_dts_after_send refers to the number of milliseconds to delay the
> toggle of RTS after the last bit has been _transmitted_.
>
> I agree with Alan that any adjustment to the delay to adhere to that
> meaning needs to be transparent to user-space.
>
>
>>> Are TIOCGRS485 and TIOCSRS485 documented in tty_ioctl man page? (I haven't
>>> updated my man pages in a while)
>>>
>>> As far as software vs. hardware and a query api, what I care about is
>>> conveying to userspace whether the implementation will be adequate for purpose,
>>> with the main issue being the true delay from actual EOT to RTS toggle
>>> when delay_after_rts_send == 0.
>>
>> Or I just can internally add (tx fifo size / baudrate) to the user
>> supplied value to take care of the bytes in tx fifo.
>
> Yes. Or poll every jiffy.
>
> But either will be far too coarse for many users; a delay_rts_after_send of
> 0 could still produce multi- _msec_ delays when the application expects
> turnaround of ~1 char time. At a leisurely 19200 baud, that's ~520us which will
> not be possible with this emulation.

If we want real-time, then we have to spin on LSR waiting for TXSRE be 1.

>
> A couple of possibilities for improving the emulation are:
> 1) Optionally using an HR timer for sub-jiffy turnaround.
> 2) Only supporting 8250-based hardware that can be set to interrupt when
>    both tx fifo and transmitter shift register are empty.

This is to support the RS485 API with already exists in omap_serrial,
but not in 8250_omap. And OMAP does not support tx line interrupt in
UART mode. So the latter is not an option.

>
> But regardless, the driver should still advertise whether direction control
> is realtime or not (ie., software or not).
>
> Regards,
> Peter Hurley
>
>
>>> Since that delay is unbounded with software methods, I thought it made sense to
>>> indicate that with a read-only bit. Naming it something else is fine too;
>>> SER_RS485_NOT_REALTIME_EOT?
>>>
>>> A more comprehensive approach might be to add a capabilities word
>>> to struct serial_rs485 which would allow the driver to report what
>>> it supports; eg. realtime turnaround or not, etc. (Not sure if extending
>>> struct serial_rs485 is really possible; the serial core hasn't been
>>> clearing padding on the driver's behalf).
>>>
>>>> At the very least the above should be clearly explained in the
>>>> documentation and patch covering notes - and if nobody can explain those
>>>> then IMHO the flag is broken.
>>>
>>> Yep.
>>>
>>> Regards,
>>> Peter Hurley
>



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

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Matwey V. Kornilov]

2015-11-18 22:39 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
> 2015-11-18 21:33 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>> On 11/17/2015 03:20 AM, Matwey V. Kornilov wrote:
>>> 2015-11-16 22:18 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>> On 11/14/2015 10:25 AM, One Thousand Gnomes wrote:
>>>>>> I specifically asked for it.
>>>>>>
>>>>>> I can think of 2 reasons that userspace wants to know:
>>>>>> 1. Because the characteristics of the software emulation are unacceptable so
>>>>>>    the application wants to terminate w/error rather than continue.
>>>>>
>>>>> But that could equally be true of hardware.
>>>>
>>>> I had this exact same thought, but concluded that it argues for a way
>>>> to select the software implementation even when h/w supports RS485.
>>>>
>>>>> In fact your software
>>>>> emulation is going to behave vastly better than many of the hardware ones.
>>>>>
>>>>>> 2. Because userspace will use different values for h/w vs. s/w. For example,
>>>>>>    right now, the emulation will raise/lower RTS prematurely when tx ends if
>>>>>>    the rts-after-send timer is 0.
>>>>>
>>>>> That's a bug then. It should be fixed as part of the merge or future
>>>>> patches - if they are not providing that emulation then they ought to do
>>>>> so and at least adjust the timing based on the baud rate so you don't
>>>>> have to spin polling the 16x50 uart to check the last bit fell out of the
>>>>> register.
>>>>
>>>> I suppose the timer(s) could be fudged and then TEMT polled (or polled every
>>>> char baud cycles). But I don't see how this will behave better than a h/w
>>>> implementation; the granularity of the jiffy clock alone will guarantee
>>>> sub-optimal turnaround, even at 9600.
>>>>
>>>>> I'd have no problem with an API that was about asking what features are
>>>>> available : both hardware and software - but the software flag seems to
>>>>> make no sense at all. Software doesn't imply anything about quality or
>>>>> feature set. If there is something the emulation cannot support then
>>>>> there should be a flag indicating that feature is not supported, not a
>>>>> flag saying software (which means nothing - as it may be supported in
>>>>> future, or may differ by uart etc).
>>>>
>>>> Fair enough.
>>>>
>>>>> It's also not "easy to drop". If it ever goes in we are stuck with a
>>>>> pointless impossible to correctly set flag for all eternity.
>>>>>
>>>>> Please explain the correct setting for this flag when a device driver
>>>>> uses hardware or software or a mix according to what the silicon is
>>>>> capable of and what values are requested ? How will an application use the
>>>>> flag meaningfully. Please explain what will happen if someone discovers a
>>>>> silicon bug and in a future 4.x release turns an implementation from
>>>>> hardware to software - will they have to lie about the flag to avoid
>>>>> breaking their application code - that strikes me as a bad thing.
>>>>
>>>> The existing driver behavior is already significantly variant and needs
>>>> to be converged, which shouldn't be too difficult. Here's a quick summary:
>>>>
>>>> mcfuart         ignores delay values, delays unsupported
>>>> imx             clamps delay values to 0, delays unsupported
>>>> atmel           only delay_rts_after_send used; delay_rts_before_send does nothing
>>>> 8250_fintek     clamps delay values to 1, unclear if h/w delay is msecs
>>>> omap-serial*    software emulation (but tx empty polling not reqd)
>>>> lpc18xx-uart    clamps delay_rts_before_send to 0, unsupported
>>>>                 clamps delay_rts_after_send to max h/w value
>>>> max310x         returns -ERANGE if either delay value > h/w support (15 msecs)
>>>> sc16is7xx*      returns -EINVAL if delay_rts_after_send is set
>>>> crisv10*        clamps delay_rts_before_send to 1000 msecs
>>>>                 ignores delays_rts_after_send (after dma is delayed by 2 * chars)
>>>> * implements delay(s) in software
>>>>
>>>> The omap-serial emulation should not have been merged in its current form.
>>>>
>>>> IMO the proper driver behavior should be clamp to h/w limit so an application
>>>> can determine the maximum delay supported. If a delay is unsupported, it should
>>>> be clamped to 0. The application should check the RS485 settings returned by
>>>> TIOCSRS485 to determine how the driver set them.
>>>> [ Documentation/serial/serial-rs485.txt should suggest/model this action ]
>>>
>>> But the similar could be true for minimal supported delay. If user
>>> requires delay which is less than lower bound, the delay is raised to
>>> the lower bound. If user requires delay which is greater than upper
>>> bound, the delay is set to the upper bound. Then software
>>> implementation could use (tx fifo size / baudrate) as lower bound for
>>> delay_after_send.
>>
>> From the application point-of-view (really the only relevant semantics),
>> delay_dts_after_send refers to the number of milliseconds to delay the
>> toggle of RTS after the last bit has been _transmitted_.
>>
>> I agree with Alan that any adjustment to the delay to adhere to that
>> meaning needs to be transparent to user-space.
>>
>>
>>>> Are TIOCGRS485 and TIOCSRS485 documented in tty_ioctl man page? (I haven't
>>>> updated my man pages in a while)
>>>>
>>>> As far as software vs. hardware and a query api, what I care about is
>>>> conveying to userspace whether the implementation will be adequate for purpose,
>>>> with the main issue being the true delay from actual EOT to RTS toggle
>>>> when delay_after_rts_send == 0.
>>>
>>> Or I just can internally add (tx fifo size / baudrate) to the user
>>> supplied value to take care of the bytes in tx fifo.
>>
>> Yes. Or poll every jiffy.
>>
>> But either will be far too coarse for many users; a delay_rts_after_send of
>> 0 could still produce multi- _msec_ delays when the application expects
>> turnaround of ~1 char time. At a leisurely 19200 baud, that's ~520us which will
>> not be possible with this emulation.
>
> If we want real-time, then we have to spin on LSR waiting for TXSRE be 1.
>
>>
>> A couple of possibilities for improving the emulation are:
>> 1) Optionally using an HR timer for sub-jiffy turnaround.
>> 2) Only supporting 8250-based hardware that can be set to interrupt when
>>    both tx fifo and transmitter shift register are empty.
>
> This is to support the RS485 API with already exists in omap_serrial,
> but not in 8250_omap. And OMAP does not support tx line interrupt in
> UART mode. So the latter is not an option.

Oh, I am sorry, it does support. There is "Supplementary Control
Register" described in 19.5.1.39

>
>>
>> But regardless, the driver should still advertise whether direction control
>> is realtime or not (ie., software or not).
>>
>> Regards,
>> Peter Hurley
>>
>>
>>>> Since that delay is unbounded with software methods, I thought it made sense to
>>>> indicate that with a read-only bit. Naming it something else is fine too;
>>>> SER_RS485_NOT_REALTIME_EOT?
>>>>
>>>> A more comprehensive approach might be to add a capabilities word
>>>> to struct serial_rs485 which would allow the driver to report what
>>>> it supports; eg. realtime turnaround or not, etc. (Not sure if extending
>>>> struct serial_rs485 is really possible; the serial core hasn't been
>>>> clearing padding on the driver's behalf).
>>>>
>>>>> At the very least the above should be clearly explained in the
>>>>> documentation and patch covering notes - and if nobody can explain those
>>>>> then IMHO the flag is broken.
>>>>
>>>> Yep.
>>>>
>>>> Regards,
>>>> Peter Hurley
>>
>
>
>
> --
> With best regards,
> Matwey V. Kornilov.
> Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
> 119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382



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

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Peter Hurley]

On 11/18/2015 02:49 PM, Matwey V. Kornilov wrote:
> 2015-11-18 22:39 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
>> 2015-11-18 21:33 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>> On 11/17/2015 03:20 AM, Matwey V. Kornilov wrote:
>>>> 2015-11-16 22:18 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>>> On 11/14/2015 10:25 AM, One Thousand Gnomes wrote:
[...]
>>>>>> It's also not "easy to drop". If it ever goes in we are stuck with a
>>>>>> pointless impossible to correctly set flag for all eternity.
>>>>>>
>>>>>> Please explain the correct setting for this flag when a device driver
>>>>>> uses hardware or software or a mix according to what the silicon is
>>>>>> capable of and what values are requested ? How will an application use the
>>>>>> flag meaningfully. Please explain what will happen if someone discovers a
>>>>>> silicon bug and in a future 4.x release turns an implementation from
>>>>>> hardware to software - will they have to lie about the flag to avoid
>>>>>> breaking their application code - that strikes me as a bad thing.
>>>>>
>>>>> The existing driver behavior is already significantly variant and needs
>>>>> to be converged, which shouldn't be too difficult. Here's a quick summary:
>>>>>
>>>>> mcfuart         ignores delay values, delays unsupported
>>>>> imx             clamps delay values to 0, delays unsupported
>>>>> atmel           only delay_rts_after_send used; delay_rts_before_send does nothing
>>>>> 8250_fintek     clamps delay values to 1, unclear if h/w delay is msecs
>>>>> omap-serial*    software emulation (but tx empty polling not reqd)
>>>>> lpc18xx-uart    clamps delay_rts_before_send to 0, unsupported
>>>>>                 clamps delay_rts_after_send to max h/w value
>>>>> max310x         returns -ERANGE if either delay value > h/w support (15 msecs)
>>>>> sc16is7xx*      returns -EINVAL if delay_rts_after_send is set
>>>>> crisv10*        clamps delay_rts_before_send to 1000 msecs
>>>>>                 ignores delays_rts_after_send (after dma is delayed by 2 * chars)
>>>>> * implements delay(s) in software
>>>>>
>>>>> The omap-serial emulation should not have been merged in its current form.
>>>>>
>>>>> IMO the proper driver behavior should be clamp to h/w limit so an application
>>>>> can determine the maximum delay supported. If a delay is unsupported, it should
>>>>> be clamped to 0. The application should check the RS485 settings returned by
>>>>> TIOCSRS485 to determine how the driver set them.
>>>>> [ Documentation/serial/serial-rs485.txt should suggest/model this action ]
>>>>
>>>> But the similar could be true for minimal supported delay. If user
>>>> requires delay which is less than lower bound, the delay is raised to
>>>> the lower bound. If user requires delay which is greater than upper
>>>> bound, the delay is set to the upper bound. Then software
>>>> implementation could use (tx fifo size / baudrate) as lower bound for
>>>> delay_after_send.
>>>
>>> From the application point-of-view (really the only relevant semantics),
>>> delay_dts_after_send refers to the number of milliseconds to delay the
>>> toggle of RTS after the last bit has been _transmitted_.

Is there consensus then about what the semantics of unsupported RS485 delay
values are? I (or someone else) can trivially add the documentation and
fixes to the existing in-tree drivers.


>>> A couple of possibilities for improving the emulation are:
>>> 1) Optionally using an HR timer for sub-jiffy turnaround.
>>> 2) Only supporting 8250-based hardware that can be set to interrupt when
>>>    both tx fifo and transmitter shift register are empty.
>>
>> This is to support the RS485 API with already exists in omap_serrial,
>> but not in 8250_omap. And OMAP does not support tx line interrupt in
>> UART mode. So the latter is not an option.
> 
> Oh, I am sorry, it does support. There is "Supplementary Control
> Register" described in 19.5.1.39

For the moment then, can we add a UART_CAP_SW485 (not exposed to userspace)
that enables this algorithm only for h/w that supports a both-empty interrupt
mode. The probe or driver (ala 8250_omap) would opt-in and configure the h/w much
like the omap-serial driver does now (with the SCR register).

Does that seem like an acceptable compromise?

Regards,
Peter Hurley

PS - I still need to review this series for how the timer logic works esp. wrt
teardown.

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Matwey V. Kornilov]

2015-12-03 2:20 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> On 11/18/2015 02:49 PM, Matwey V. Kornilov wrote:
>> 2015-11-18 22:39 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
>>> 2015-11-18 21:33 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>> On 11/17/2015 03:20 AM, Matwey V. Kornilov wrote:
>>>>> 2015-11-16 22:18 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>>>> On 11/14/2015 10:25 AM, One Thousand Gnomes wrote:
> [...]
>>>>>>> It's also not "easy to drop". If it ever goes in we are stuck with a
>>>>>>> pointless impossible to correctly set flag for all eternity.
>>>>>>>
>>>>>>> Please explain the correct setting for this flag when a device driver
>>>>>>> uses hardware or software or a mix according to what the silicon is
>>>>>>> capable of and what values are requested ? How will an application use the
>>>>>>> flag meaningfully. Please explain what will happen if someone discovers a
>>>>>>> silicon bug and in a future 4.x release turns an implementation from
>>>>>>> hardware to software - will they have to lie about the flag to avoid
>>>>>>> breaking their application code - that strikes me as a bad thing.
>>>>>>
>>>>>> The existing driver behavior is already significantly variant and needs
>>>>>> to be converged, which shouldn't be too difficult. Here's a quick summary:
>>>>>>
>>>>>> mcfuart         ignores delay values, delays unsupported
>>>>>> imx             clamps delay values to 0, delays unsupported
>>>>>> atmel           only delay_rts_after_send used; delay_rts_before_send does nothing
>>>>>> 8250_fintek     clamps delay values to 1, unclear if h/w delay is msecs
>>>>>> omap-serial*    software emulation (but tx empty polling not reqd)
>>>>>> lpc18xx-uart    clamps delay_rts_before_send to 0, unsupported
>>>>>>                 clamps delay_rts_after_send to max h/w value
>>>>>> max310x         returns -ERANGE if either delay value > h/w support (15 msecs)
>>>>>> sc16is7xx*      returns -EINVAL if delay_rts_after_send is set
>>>>>> crisv10*        clamps delay_rts_before_send to 1000 msecs
>>>>>>                 ignores delays_rts_after_send (after dma is delayed by 2 * chars)
>>>>>> * implements delay(s) in software
>>>>>>
>>>>>> The omap-serial emulation should not have been merged in its current form.
>>>>>>
>>>>>> IMO the proper driver behavior should be clamp to h/w limit so an application
>>>>>> can determine the maximum delay supported. If a delay is unsupported, it should
>>>>>> be clamped to 0. The application should check the RS485 settings returned by
>>>>>> TIOCSRS485 to determine how the driver set them.
>>>>>> [ Documentation/serial/serial-rs485.txt should suggest/model this action ]
>>>>>
>>>>> But the similar could be true for minimal supported delay. If user
>>>>> requires delay which is less than lower bound, the delay is raised to
>>>>> the lower bound. If user requires delay which is greater than upper
>>>>> bound, the delay is set to the upper bound. Then software
>>>>> implementation could use (tx fifo size / baudrate) as lower bound for
>>>>> delay_after_send.
>>>>
>>>> From the application point-of-view (really the only relevant semantics),
>>>> delay_dts_after_send refers to the number of milliseconds to delay the
>>>> toggle of RTS after the last bit has been _transmitted_.
>
> Is there consensus then about what the semantics of unsupported RS485 delay
> values are? I (or someone else) can trivially add the documentation and
> fixes to the existing in-tree drivers.
>
>
>>>> A couple of possibilities for improving the emulation are:
>>>> 1) Optionally using an HR timer for sub-jiffy turnaround.
>>>> 2) Only supporting 8250-based hardware that can be set to interrupt when
>>>>    both tx fifo and transmitter shift register are empty.
>>>
>>> This is to support the RS485 API with already exists in omap_serrial,
>>> but not in 8250_omap. And OMAP does not support tx line interrupt in
>>> UART mode. So the latter is not an option.
>>
>> Oh, I am sorry, it does support. There is "Supplementary Control
>> Register" described in 19.5.1.39
>
> For the moment then, can we add a UART_CAP_SW485 (not exposed to userspace)
> that enables this algorithm only for h/w that supports a both-empty interrupt
> mode. The probe or driver (ala 8250_omap) would opt-in and configure the h/w much
> like the omap-serial driver does now (with the SCR register).
>
> Does that seem like an acceptable compromise?
>
> Regards,
> Peter Hurley
>
> PS - I still need to review this series for how the timer logic works esp. wrt
> teardown.
>

Dear Peter,

I am working on v4, where I completely redesigned implementation. And
now I think that it is considerably better than v3.
It looks like the following:
https://github.com/matwey/linux/commits/8520_rs485_v4
But it is not ready yet, there is a bug somewhere.

In the v4, each subdriver decides separately if it needs rs485
emulation support. Then it enables it like the following:
https://github.com/matwey/linux/commit/4455e425fc045713fb921ccec695fe183f1558f0
Before calling serial8250_rs485_emul_enabled, the driver enables
interrupt on empty shift register (they are always there for omap_).

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

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Peter Hurley]

Hi Matwey,

On 12/03/2015 12:50 AM, Matwey V. Kornilov wrote:
> I am working on v4, where I completely redesigned implementation. And
> now I think that it is considerably better than v3.
> It looks like the following:
> https://github.com/matwey/linux/commits/8520_rs485_v4
> But it is not ready yet, there is a bug somewhere.
> 
> In the v4, each subdriver decides separately if it needs rs485
> emulation support. Then it enables it like the following:
> https://github.com/matwey/linux/commit/4455e425fc045713fb921ccec695fe183f1558f0
> Before calling serial8250_rs485_emul_enabled, the driver enables
> interrupt on empty shift register (they are always there for omap_).
 
Looks good.

Are you testing with CONFIG_SERIAL_8250_DMA=n first to simplify the
debug effort? DMA adds a completely different tx path.

Also, before submission, please shorten the identifiers. And Greg hates
functions returning bool so just expanded serial8250_rs485_emul_enabled()
inline.

Regards,
Peter Hurley

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Matwey V. Kornilov]

2015-12-03 17:41 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> Hi Matwey,
>
> On 12/03/2015 12:50 AM, Matwey V. Kornilov wrote:
>> I am working on v4, where I completely redesigned implementation. And
>> now I think that it is considerably better than v3.
>> It looks like the following:
>> https://github.com/matwey/linux/commits/8520_rs485_v4
>> But it is not ready yet, there is a bug somewhere.
>>
>> In the v4, each subdriver decides separately if it needs rs485
>> emulation support. Then it enables it like the following:
>> https://github.com/matwey/linux/commit/4455e425fc045713fb921ccec695fe183f1558f0
>> Before calling serial8250_rs485_emul_enabled, the driver enables
>> interrupt on empty shift register (they are always there for omap_).
>
> Looks good.
>
> Are you testing with CONFIG_SERIAL_8250_DMA=n first to simplify the
> debug effort? DMA adds a completely different tx path.

Many thanks for the advice. I've just found that the bug is not in my code =)
Even with pure 4.3.0 I cannot open /dev/ttyS5 more than once. It just
hangs on open() and the process is in S+ state.

>
> Also, before submission, please shorten the identifiers. And Greg hates
> functions returning bool so just expanded serial8250_rs485_emul_enabled()
> inline.

Am I allowed to use `re' instead of rs485_emul in names?

>
> Regards,
> Peter Hurley
>



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

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Peter Hurley]

On 12/03/2015 12:29 PM, Matwey V. Kornilov wrote:
> 2015-12-03 17:41 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>> Hi Matwey,
>>
>> On 12/03/2015 12:50 AM, Matwey V. Kornilov wrote:
>>> I am working on v4, where I completely redesigned implementation. And
>>> now I think that it is considerably better than v3.
>>> It looks like the following:
>>> https://github.com/matwey/linux/commits/8520_rs485_v4
>>> But it is not ready yet, there is a bug somewhere.
>>>
>>> In the v4, each subdriver decides separately if it needs rs485
>>> emulation support. Then it enables it like the following:
>>> https://github.com/matwey/linux/commit/4455e425fc045713fb921ccec695fe183f1558f0
>>> Before calling serial8250_rs485_emul_enabled, the driver enables
>>> interrupt on empty shift register (they are always there for omap_).
>>
>> Looks good.
>>
>> Are you testing with CONFIG_SERIAL_8250_DMA=n first to simplify the
>> debug effort? DMA adds a completely different tx path.
> 
> Many thanks for the advice. I've just found that the bug is not in my code =)
> Even with pure 4.3.0 I cannot open /dev/ttyS5 more than once. It just
> hangs on open() and the process is in S+ state.

Hmm, that's odd. So

$ stty -a < /dev/ttyS5

hangs if something like below is running?

$ cat > /dev/ttyS5


>> Also, before submission, please shorten the identifiers. And Greg hates
>> functions returning bool so just expanded serial8250_rs485_emul_enabled()
>> inline.
> 
> Am I allowed to use `re' instead of rs485_emul in names?

Long names and constructs tend to obscure the execution flow.
Some of the names could be reduced where the meaning is obvious:

  serial8250_rts_on_send
  serial8250_rts_after_send
  serial8250_handle_start_timer
  serial8250_handle_stop_timer

These two I would inline into their lone call site:

  serial8250_rs485_emul_startup()
  serial8250_rs485_emul_shutdown()

serial8250_rs485_emul_start_tx  => __start_tx_rs485

rs485_emul => sw485/em485/emul485/soft485 ?

Or just rs485 (except for the field name and structs so as not to confuse
it with the port->rs485)

Just my 2¢

Regards,
Peter Hurley

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

[Matwey V. Kornilov]

2015-12-03 22:45 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> On 12/03/2015 12:29 PM, Matwey V. Kornilov wrote:
>> 2015-12-03 17:41 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>> Hi Matwey,
>>>
>>> On 12/03/2015 12:50 AM, Matwey V. Kornilov wrote:
>>>> I am working on v4, where I completely redesigned implementation. And
>>>> now I think that it is considerably better than v3.
>>>> It looks like the following:
>>>> https://github.com/matwey/linux/commits/8520_rs485_v4
>>>> But it is not ready yet, there is a bug somewhere.
>>>>
>>>> In the v4, each subdriver decides separately if it needs rs485
>>>> emulation support. Then it enables it like the following:
>>>> https://github.com/matwey/linux/commit/4455e425fc045713fb921ccec695fe183f1558f0
>>>> Before calling serial8250_rs485_emul_enabled, the driver enables
>>>> interrupt on empty shift register (they are always there for omap_).
>>>
>>> Looks good.
>>>
>>> Are you testing with CONFIG_SERIAL_8250_DMA=n first to simplify the
>>> debug effort? DMA adds a completely different tx path.
>>
>> Many thanks for the advice. I've just found that the bug is not in my code =)
>> Even with pure 4.3.0 I cannot open /dev/ttyS5 more than once. It just
>> hangs on open() and the process is in S+ state.
>
> Hmm, that's odd. So
>
> $ stty -a < /dev/ttyS5
>
> hangs if something like below is running?
>
> $ cat > /dev/ttyS5
>

Nonblocking mode works, blocking mode hands on tty_port_block_til_ready
https://bugzilla.kernel.org/show_bug.cgi?id=108851

>
>>> Also, before submission, please shorten the identifiers. And Greg hates
>>> functions returning bool so just expanded serial8250_rs485_emul_enabled()
>>> inline.

I would like to keep it as API to hide implementation details.
In other words, I don't want to inline it in omap_8250_rs485_config.

>>
>> Am I allowed to use `re' instead of rs485_emul in names?
>
> Long names and constructs tend to obscure the execution flow.
> Some of the names could be reduced where the meaning is obvious:
>
>   serial8250_rts_on_send
>   serial8250_rts_after_send
>   serial8250_handle_start_timer
>   serial8250_handle_stop_timer
>
> These two I would inline into their lone call site:
>
>   serial8250_rs485_emul_startup()
>   serial8250_rs485_emul_shutdown()
>
> serial8250_rs485_emul_start_tx  => __start_tx_rs485
>
> rs485_emul => sw485/em485/emul485/soft485 ?
>
> Or just rs485 (except for the field name and structs so as not to confuse
> it with the port->rs485)
>
> Just my 2¢
>
> Regards,
> Peter Hurley
>
>



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