[PATCH v7 0/3] tty: Introduce software RS485 direction control support

[PATCH v7 0/3] tty: Introduce software RS485 direction control support

[Matwey V. Kornilov]

Changes from v6:
 - minor style changes
 - timers are not IRQSAFE now
Changes from v5:
 - rs485_emul variable has been renamed to em485 to follow function names convention
Changes from v4:
 - Add commit message to 1/3
Changes from v3:
 - Completely redesigned.
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

Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>

[PATCH v7 1/3] tty: Move serial8250_stop_rx in front of serial8250_start_tx

[Matwey V. Kornilov]

Software RS485 emultaion is to be added in the following commit.
serial8250_start_tx will need to refer serial8250_stop_rx.
Move serial8250_stop_rx in front of serial8250_start_tx in order
to avoid function forward declaration.

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 8d262bc..b82fcae 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1304,6 +1304,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) {
@@ -1371,19 +1384,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.7.0

[PATCH v7 2/3] 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.

Note that before calling serial8250_em485_init the caller has to
ensure that UART will interrupt when shift register empty. Otherwise,
emultaion cannot be used.

Both serial8250_em485_init and serial8250_em485_destroy are
idempotent functions.

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

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index d54dcd8..ce587c0 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -117,6 +117,8 @@ static inline void serial_dl_write(struct uart_8250_port *up, int value)
 struct uart_8250_port *serial8250_get_port(int line);
 void serial8250_rpm_get(struct uart_8250_port *p);
 void serial8250_rpm_put(struct uart_8250_port *p);
+int serial8250_em485_init(struct uart_8250_port *p);
+void serial8250_em485_destroy(struct uart_8250_port *p);
 
 #if defined(__alpha__) && !defined(CONFIG_PCI)
 /*
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index b82fcae..f9442e8 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>
@@ -522,6 +523,20 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
 	}
 }
 
+static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
+{
+	unsigned char mcr = serial_in(p, UART_MCR);
+
+	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
+		mcr |= UART_MCR_RTS;
+	else
+		mcr &= ~UART_MCR_RTS;
+	serial_out(p, UART_MCR, mcr);
+}
+
+static void serial8250_em485_handle_start_tx(unsigned long arg);
+static void serial8250_em485_handle_stop_tx(unsigned long arg);
+
 void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
 {
 	serial8250_clear_fifos(p);
@@ -546,6 +561,72 @@ void serial8250_rpm_put(struct uart_8250_port *p)
 }
 EXPORT_SYMBOL_GPL(serial8250_rpm_put);
 
+/**
+ *	serial8250_em485_init - put uart_8250_port into rs485 emulating
+ *	@p: uart_8250_port port instance
+ *
+ *	The function is used to start rs485 software emulating on the
+ *	uart_8250_port @p. Namely, RTS is switched before/after transmission.
+ *	The function is idempotent, so it is safe to call it multiple times.
+ *
+ *	The caller MUST enable interrupt on empty shift register before
+ *	calling serial8250_em485_init. This interrupt is not a part of
+ *	8250 standard, but implementation defined.
+ *
+ *	The function is supposed to be called from .rs485_config callback
+ *	or from any other callback protected with p->port.lock spinlock.
+ *
+ *	The return value is negative error code or 0 on succes.
+ *
+ *	See also serial8250_em485_destroy
+ */
+int serial8250_em485_init(struct uart_8250_port *p)
+{
+	if (p->em485 != NULL)
+		return 0;
+
+	p->em485 = kmalloc(sizeof(struct uart_8250_em485), GFP_KERNEL);
+	if (p->em485 == NULL)
+		return -ENOMEM;
+
+	setup_timer(&p->em485->stop_tx_timer,
+		serial8250_em485_handle_stop_tx, (unsigned long)p);
+	setup_timer(&p->em485->start_tx_timer,
+		serial8250_em485_handle_start_tx, (unsigned long)p);
+	p->em485->active_timer = NULL;
+
+	serial8250_em485_rts_after_send(p);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(serial8250_em485_init);
+
+/**
+ *	serial8250_em485_destroy - put uart_8250_port into normal state
+ *	@p: uart_8250_port port instance
+ *
+ *	The function is used to stop rs485 software emulating on the
+ *	uart_8250_port @p. The function is idempotent, so it is safe to
+ *	call it multiple times.
+ *
+ *	The function is supposed to be called from rs485_config callback
+ *	or from any other callback protected with p->port.lock spinlock.
+ *
+ *	See also serial8250_em485_init
+ */
+void serial8250_em485_destroy(struct uart_8250_port *p)
+{
+	if (p->em485 == NULL)
+		return;
+
+	del_timer(&p->em485->start_tx_timer);
+	del_timer(&p->em485->stop_tx_timer);
+
+	kfree(p->em485);
+	p->em485 = NULL;
+}
+EXPORT_SYMBOL_GPL(serial8250_em485_destroy);
+
 /*
  * These two wrappers ensure that enable_runtime_pm_tx() can be called more than
  * once and disable_runtime_pm_tx() will still disable RPM because the fifo is
@@ -1317,7 +1398,52 @@ static void serial8250_stop_rx(struct uart_port *port)
 	serial8250_rpm_put(up);
 }
 
-static inline void __stop_tx(struct uart_8250_port *p)
+static void __do_stop_tx_rs485(struct uart_8250_port *p)
+{
+	if (!p->em485)
+		return;
+
+	serial8250_em485_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_em485_handle_stop_tx(unsigned long arg)
+{
+	struct uart_8250_port *p = (struct uart_8250_port *)arg;
+	unsigned long flags;
+
+	spin_lock_irqsave(&p->port.lock, flags);
+	if (p->em485 &&
+	    p->em485->active_timer == &p->em485->stop_tx_timer) {
+		__do_stop_tx_rs485(p);
+		p->em485->active_timer = NULL;
+	}
+	spin_unlock_irqrestore(&p->port.lock, flags);
+}
+
+static void __stop_tx_rs485(struct uart_8250_port *p)
+{
+	if (!p->em485)
+		return;
+
+	/* __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) {
+		p->em485->active_timer = &p->em485->stop_tx_timer;
+		mod_timer(&p->em485->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;
@@ -1326,6 +1452,24 @@ static inline void __stop_tx(struct uart_8250_port *p)
 	}
 }
 
+static inline void __stop_tx(struct uart_8250_port *p)
+{
+	if (p->em485) {
+		unsigned char lsr = serial_in(p, UART_LSR);
+		/* To provide required timeing and allow FIFO transfer,
+		 * __stop_tx_rs485 must be called only when both FIFO and shift register
+		 * are empty. It is for device driver to enable interrupt on TEMT.
+		 */
+		if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
+			return;
+
+		del_timer(&p->em485->start_tx_timer);
+		p->em485->active_timer = NULL;
+	}
+	__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);
@@ -1343,12 +1487,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;
 
@@ -1374,6 +1516,67 @@ static void serial8250_start_tx(struct uart_port *port)
 	}
 }
 
+static inline void start_tx_rs485(struct uart_port *port)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	unsigned char mcr;
+
+	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
+		serial8250_stop_rx(&up->port);
+
+	del_timer(&up->em485->stop_tx_timer);
+	up->em485->active_timer = NULL;
+
+	mcr = serial_in(up, UART_MCR);
+	if (!!(up->port.rs485.flags & SER_RS485_RTS_ON_SEND) !=
+	    !!(mcr & UART_MCR_RTS)) {
+		if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
+			mcr |= UART_MCR_RTS;
+		else
+			mcr &= ~UART_MCR_RTS;
+		serial_out(up, UART_MCR, mcr);
+
+		if (up->port.rs485.delay_rts_before_send > 0) {
+			up->em485->active_timer = &up->em485->start_tx_timer;
+			mod_timer(&up->em485->start_tx_timer, jiffies +
+				up->port.rs485.delay_rts_before_send * HZ / 1000);
+			return;
+		}
+	}
+
+	__start_tx(port);
+}
+
+static void serial8250_em485_handle_start_tx(unsigned long arg)
+{
+	struct uart_8250_port *p = (struct uart_8250_port *)arg;
+	unsigned long flags;
+
+	spin_lock_irqsave(&p->port.lock, flags);
+	if (p->em485 &&
+	    p->em485->active_timer == &p->em485->start_tx_timer) {
+		__start_tx(&p->port);
+		p->em485->active_timer = NULL;
+	}
+	spin_unlock_irqrestore(&p->port.lock, flags);
+}
+
+static void serial8250_start_tx(struct uart_port *port)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+
+	serial8250_rpm_get_tx(up);
+
+	if (up->em485 &&
+	    up->em485->active_timer == &up->em485->start_tx_timer)
+		return;
+
+	if (up->em485)
+		start_tx_rs485(port);
+	else
+		__start_tx(port);
+}
+
 static void serial8250_throttle(struct uart_port *port)
 {
 	port->throttle(port);
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index faa0e03..4348797 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -76,6 +76,12 @@ struct uart_8250_ops {
 	void		(*release_irq)(struct uart_8250_port *);
 };
 
+struct uart_8250_em485 {
+	struct timer_list	start_tx_timer; /* "rs485 start tx" timer */
+	struct timer_list	stop_tx_timer;  /* "rs485 stop tx" timer */
+	struct timer_list	*active_timer;  /* pointer to active timer */
+};
+
 /*
  * This should be used by drivers which want to register
  * their own 8250 ports without registering their own
@@ -122,6 +128,8 @@ struct uart_8250_port {
 	/* 8250 specific callbacks */
 	int			(*dl_read)(struct uart_8250_port *);
 	void			(*dl_write)(struct uart_8250_port *, int);
+
+	struct uart_8250_em485 *em485;
 };
 
 static inline struct uart_8250_port *up_to_u8250p(struct uart_port *up)
-- 
2.7.0

[PATCH v7 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Matwey V. Kornilov]

Use software emulated RS485 direction control to provide RS485 API existed in
omap_serial driver. Note that 8250_omap issues interrupt on shift register
empty which is single prerequesite for using software emulated RS485.

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

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index a2c0734..31f7905 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -697,6 +697,30 @@ static void omap_8250_throttle(struct uart_port *port)
 	pm_runtime_put_autosuspend(port->dev);
 }
 
+static int omap_8250_rs485_config(struct uart_port *port, struct serial_rs485 *rs485)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+
+	/* Clamp the delays to [0, 100ms] */
+	rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
+	rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send, 100U);
+
+	port->rs485 = *rs485;
+
+	/* Both serial8250_em485_* functions are idempotent */
+	if (rs485->flags & SER_RS485_ENABLED) {
+		int ret = serial8250_em485_init(up);
+		if (ret) {
+			rs485->flags &= ~SER_RS485_ENABLED;
+			port->rs485.flags &= ~SER_RS485_ENABLED;
+		}
+		return ret;
+	} else
+		serial8250_em485_destroy(up);
+
+	return 0;
+}
+
 static void omap_8250_unthrottle(struct uart_port *port)
 {
 	unsigned long flags;
@@ -1146,6 +1170,7 @@ static int omap8250_probe(struct platform_device *pdev)
 	up.port.shutdown = omap_8250_shutdown;
 	up.port.throttle = omap_8250_throttle;
 	up.port.unthrottle = omap_8250_unthrottle;
+	up.port.rs485_config = omap_8250_rs485_config;
 
 	if (pdev->dev.of_node) {
 		const struct of_device_id *id;
-- 
2.7.0

Re: [PATCH v7 2/3] tty: Add software emulated RS485 support for 8250

[Andy Shevchenko]

On Tue, Jan 19, 2016 at 10: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.
>
> Note that before calling serial8250_em485_init the caller has to
> ensure that UART will interrupt when shift register empty. Otherwise,
> emultaion cannot be used.
>
> Both serial8250_em485_init and serial8250_em485_destroy are
> idempotent functions.

Seems you have issues with kernel-doc and commentary styles.

Functions in kernel doc are marked as func(), so don't forget parens.

Multi-line comment blocks are going in the style like:

/*
 * Text line 1.
 * Line 2. New sentence.
 */

You may notice the periods at the end of sentences. There is no need
to keep more heading spaces.

> +
> +static void serial8250_em485_handle_start_tx(unsigned long arg);
> +static void serial8250_em485_handle_stop_tx(unsigned long arg);
> +

And you need forward declarations because of some cyclic dependency?

> +static void serial8250_start_tx(struct uart_port *port)
> +{
> +       struct uart_8250_port *up = up_to_u8250p(port);
> +
> +       serial8250_rpm_get_tx(up);
> +

> +       if (up->em485 &&
> +           up->em485->active_timer == &up->em485->start_tx_timer)
> +               return;

struct uart_8250_em485 *em485 = up->em485;

if (em485 && em485->active_timer == &em485->start_tx_timer)

?

Same for the rest of code.

> +
> +       if (up->em485)
> +               start_tx_rs485(port);
> +       else
> +               __start_tx(port);
> +}

> +struct uart_8250_em485 {
> +       struct timer_list       start_tx_timer;

…/* "rs485 start tx" timer */

-> kernel-doc

> +       struct timer_list       stop_tx_timer;  /* "rs485 stop tx" timer */
> +       struct timer_list       *active_timer;  /* pointer to active timer */
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Andy Shevchenko]

On Tue, Jan 19, 2016 at 10:33 PM, Matwey V. Kornilov <matwey@sai.msu.ru> wrote:
> Use software emulated RS485 direction control to provide RS485 API existed in
> omap_serial driver. Note that 8250_omap issues interrupt on shift register
> empty which is single prerequesite for using software emulated RS485.


> +static int omap_8250_rs485_config(struct uart_port *port, struct serial_rs485 *rs485)
> +{
> +       struct uart_8250_port *up = up_to_u8250p(port);
> +
> +       /* Clamp the delays to [0, 100ms] */

It's not exactly clamping but rather cutting extra as I can see below.

> +       rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
> +       rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send, 100U);

> +       /* Both serial8250_em485_* functions are idempotent */

Better to decode what functions do you mean here.

> +       if (rs485->flags & SER_RS485_ENABLED) {
> +               int ret = serial8250_em485_init(up);
> +               if (ret) {
> +                       rs485->flags &= ~SER_RS485_ENABLED;
> +                       port->rs485.flags &= ~SER_RS485_ENABLED;
> +               }

> +               return ret;
> +       } else

You have to do } else { in multi-line branches, but luckily there is
redundant else keyword.

And checkpatch.pl doesn't tell you anything?

> +               serial8250_em485_destroy(up);
> +
> +       return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Matwey V. Kornilov]

2016-01-23 18:20 GMT+03:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Tue, Jan 19, 2016 at 10:33 PM, Matwey V. Kornilov <matwey@sai.msu.ru> wrote:
>> Use software emulated RS485 direction control to provide RS485 API existed in
>> omap_serial driver. Note that 8250_omap issues interrupt on shift register
>> empty which is single prerequesite for using software emulated RS485.
>
>
>> +static int omap_8250_rs485_config(struct uart_port *port, struct serial_rs485 *rs485)
>> +{
>> +       struct uart_8250_port *up = up_to_u8250p(port);
>> +
>> +       /* Clamp the delays to [0, 100ms] */
>
> It's not exactly clamping but rather cutting extra as I can see below.
>

I took this code and the comment from
http://marc.info/?l=linux-serial&m=145264055108439&w=2

>> +       rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
>> +       rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send, 100U);
>
>> +       /* Both serial8250_em485_* functions are idempotent */
>
> Better to decode what functions do you mean here.
>
>> +       if (rs485->flags & SER_RS485_ENABLED) {
>> +               int ret = serial8250_em485_init(up);
>> +               if (ret) {
>> +                       rs485->flags &= ~SER_RS485_ENABLED;
>> +                       port->rs485.flags &= ~SER_RS485_ENABLED;
>> +               }
>
>> +               return ret;
>> +       } else
>
> You have to do } else { in multi-line branches, but luckily there is
> redundant else keyword.
>
> And checkpatch.pl doesn't tell you anything?
>
>> +               serial8250_em485_destroy(up);
>> +
>> +       return 0;
>> +}
>
> --
> With Best Regards,
> Andy Shevchenko
>



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