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

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

[Matwey V. Kornilov]

Changes from v7:
 - rework comments to follow guidelines
 - minor style changes
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 v8 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 v8 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 | 223 +++++++++++++++++++++++++++++++++++-
 include/linux/serial_8250.h         |   8 ++
 3 files changed, 229 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..c908b77 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,73 @@ 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
+ *	&struct uart_8250_port* @p. Namely, RTS is switched before/after
+ *	transmission. The function is idempotent, so it is safe to call it
+ *	multiple times.
+ *
+ *	The caller MUST enable interrupt on empty shift register before
+ *	calling serial8250_em485_init(). This interrupt is not a part of
+ *	8250 standard, but implementation defined.
+ *
+ *	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_destroy()
+ *
+ *	Return 0 - success, -errno - otherwise
+ */
+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
+ *	&struct 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 +1399,56 @@ 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;
+	struct uart_8250_em485 *em485 = p->em485;
+	unsigned long flags;
+
+	spin_lock_irqsave(&p->port.lock, flags);
+	if (em485 &&
+	    em485->active_timer == &em485->stop_tx_timer) {
+		__do_stop_tx_rs485(p);
+		em485->active_timer = NULL;
+	}
+	spin_unlock_irqrestore(&p->port.lock, flags);
+}
+
+static void __stop_tx_rs485(struct uart_8250_port *p)
+{
+	struct uart_8250_em485 *em485 = p->em485;
+
+	if (!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) {
+		em485->active_timer = &em485->stop_tx_timer;
+		mod_timer(&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 +1457,28 @@ static inline void __stop_tx(struct uart_8250_port *p)
 	}
 }
 
+static inline void __stop_tx(struct uart_8250_port *p)
+{
+	struct uart_8250_em485 *em485 = p->em485;
+
+	if (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(&em485->start_tx_timer);
+		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 +1496,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 +1525,70 @@ 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);
+	struct uart_8250_em485 *em485 = up->em485;
+	unsigned char mcr;
+
+	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
+		serial8250_stop_rx(&up->port);
+
+	del_timer(&em485->stop_tx_timer);
+	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) {
+			em485->active_timer = &em485->start_tx_timer;
+			mod_timer(&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;
+	struct uart_8250_em485 *em485 = p->em485;
+	unsigned long flags;
+
+	spin_lock_irqsave(&p->port.lock, flags);
+	if (em485 &&
+	    em485->active_timer == &em485->start_tx_timer) {
+		__start_tx(&p->port);
+		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);
+	struct uart_8250_em485 *em485 = up->em485;
+
+	serial8250_rpm_get_tx(up);
+
+	if (em485 &&
+	    em485->active_timer == &em485->start_tx_timer)
+		return;
+
+	if (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 v8 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 | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index a2c0734..d710985 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -697,6 +697,36 @@ 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_init and serial8250_em485_destroy
+	 * are idempotent
+	 */
+	if (rs485->flags & SER_RS485_ENABLED) {
+		int ret = serial8250_em485_init(up);
+
+		if (ret) {
+			rs485->flags &= ~SER_RS485_ENABLED;
+			port->rs485.flags &= ~SER_RS485_ENABLED;
+		}
+		return ret;
+	}
+
+	serial8250_em485_destroy(up);
+
+	return 0;
+}
+
 static void omap_8250_unthrottle(struct uart_port *port)
 {
 	unsigned long flags;
@@ -1146,6 +1176,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 v8 0/3] tty: Introduce software RS485 direction control support

[Peter Hurley]

Hi Matwey,

On 02/01/2016 10:09 AM, Matwey V. Kornilov wrote:
> Changes from v7:
>  - rework comments to follow guidelines
>  - minor style changes
> 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>

The holdup here is that I'd like to unit test this, which is 3rd on my todo list.
Should be done before the end of the week which will be soon enough
for this series to make linux-next.

Thanks for your work on this.

Regards,
Peter Hurley

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

[Matwey V. Kornilov]

2016-02-10 19:05 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> Hi Matwey,
>
> On 02/01/2016 10:09 AM, Matwey V. Kornilov wrote:
>> Changes from v7:
>>  - rework comments to follow guidelines
>>  - minor style changes
>> 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>
>
> The holdup here is that I'd like to unit test this, which is 3rd on my todo list.
> Should be done before the end of the week which will be soon enough
> for this series to make linux-next.

Hi Peter,

Is something required from me?

>
> Thanks for your work on this.
>
> 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 v8 0/3] tty: Introduce software RS485 direction control support

[Peter Hurley]

On 02/10/2016 08:24 AM, Matwey V. Kornilov wrote:
> 2016-02-10 19:05 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>> Hi Matwey,
>>
>> On 02/01/2016 10:09 AM, Matwey V. Kornilov wrote:
>>> Changes from v7:
>>>  - rework comments to follow guidelines
>>>  - minor style changes
>>> 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>
>>
>> The holdup here is that I'd like to unit test this, which is 3rd on my todo list.
>> Should be done before the end of the week which will be soon enough
>> for this series to make linux-next.
> 
> Hi Peter,
> 
> Is something required from me?

Not unless my tests identify a problem. I'll let you know.

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

[Ильяс Гасанов]

Hello,

2016-02-18 19:50 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> No, that's not the way.
>
> First, there's plenty of driver hooks to properly manage the RTS level
> for rs485.

For now I'm considering to add the check to uart_dtr_rts in
serial_core.c. However, given the access to struct
tty_port_operations, I think it's also possible to use a
driver-specific hook if it makes more sense. The question is - where
exactly does it belong?

> Second, how are you running GPIO rts with the omap8250 driver?

Through hooking set_mctrl and get_mctrl and using the suggested
helpers from serial_mctrl_gpio.c. Though I had to introduce
uart_port.get_mctrl to serial_core.h (with set_mctrl already being
there) and serial8250_do_get_mctrl to serial_8250.h (same here,
serial8250_do_set_mctrl is already there).


Regards,
Ilyas G.

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

[Peter Hurley]

On 02/18/2016 01:46 AM, Ильяс Гасанов wrote:
> 2016-02-18 10:18 GMT+03:00 Ильяс Гасанов <torso.nafi@gmail.com>:
>> Also, forgot to mention that if serial8250_em485_init is called not
>> upon uart startup but elsewhere (upon port register for example), and
>> em485 is set, serial8250_do_startup should call
>> serial8250_em485_rts_after_send, or else RTS might be in wrong state
>> whenever the port device is opened, making it impossible to receive
>> data through RTS-controlled RS232<->RS485 hardware converters.
> 
> Just found out that it doesn't help. Actually, it is the behavior of
> tty_open function to call tty_port_block_til_ready after startup, and
> of the latter - to set RTS via ioctl if baud is not zero - both of
> which cannot be overriden within the scope of the 8250 framework. The
> reason this works in legacy omap_serial is that my GPIO which is used
> to emulate RTS is not tied to mctrl there, so the call of
> tty_port_block_til_ready does not affect its state.
> 
> Therefore, a check for SER_RS485_ENABLED needs to be introduced to
> tty_port_block_til_ready.

No, that's not the way.

First, there's plenty of driver hooks to properly manage the RTS level
for rs485.
Second, how are you running GPIO rts with the omap8250 driver?

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

[Ильяс Гасанов]

2016-02-18 10:18 GMT+03:00 Ильяс Гасанов <torso.nafi@gmail.com>:
> Also, forgot to mention that if serial8250_em485_init is called not
> upon uart startup but elsewhere (upon port register for example), and
> em485 is set, serial8250_do_startup should call
> serial8250_em485_rts_after_send, or else RTS might be in wrong state
> whenever the port device is opened, making it impossible to receive
> data through RTS-controlled RS232<->RS485 hardware converters.

Just found out that it doesn't help. Actually, it is the behavior of
tty_open function to call tty_port_block_til_ready after startup, and
of the latter - to set RTS via ioctl if baud is not zero - both of
which cannot be overriden within the scope of the 8250 framework. The
reason this works in legacy omap_serial is that my GPIO which is used
to emulate RTS is not tied to mctrl there, so the call of
tty_port_block_til_ready does not affect its state.

Therefore, a check for SER_RS485_ENABLED needs to be introduced to
tty_port_block_til_ready.


Regards,
Ilyas G.

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

[Ильяс Гасанов]

Also, forgot to mention that if serial8250_em485_init is called not
upon uart startup but elsewhere (upon port register for example), and
em485 is set, serial8250_do_startup should call
serial8250_em485_rts_after_send, or else RTS might be in wrong state
whenever the port device is opened, making it impossible to receive
data through RTS-controlled RS232<->RS485 hardware converters.


Regards,
Ilyas G.

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

[Ильяс Гасанов]

Hello,

After testing the patch v8, I found two more glitches, which happen at
least in 4.1 kernel (I applied the changes to 8250_core.c, since
8250_port.c isn't made separate in 4.1):

1) When serial8250_em485_init is called from omap_8250_rs485_config,
there is a lockdep warning, and according to the stack trace it
happens at kmalloc invocation. Passing GFP_ATOMIC instead of
GFP_KERNEL in flags apparently fixes the problem.

2) Once a transmission happens and completes, the behavior of AM335x
UART becomes strange - it seemingly doesn't receive anything (or fails
to return to userspace the received payload). At the time of next
transmission (sometimes just before, sometimes just after, but never
during it) the single last received byte is returned via read, the
rest of the payload discarded. However when SER_RS485_RX_DURING_TX is
set, it works just okay. The problem is, this happens when clearly the
payload is not received during transmission, so it isn't liable to be
discarded upon clearing FIFO. Best guess the Rx interrupt which is
disabled in start_tx_rs485 upon calling serial8250_stop_rx isn't
properly restored.


Regards,
Ilyas G.

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

[Ильяс Гасанов]

2016-02-12 10:57 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
> up->em485 is always pointer, however you are right, that
> serial8250_em485_init stores pointer to up when the timers are inited.
> More complex issue here that serial8250_em485_init need to set RTS to
> proper state and probably can't do that before
> serial8250_register_8250_port. So omap8250_probe should directly or
> indirectly use serial8250_get_port(priv->line) after
> serial8250_register_8250_port. As for me, possible solution could be
> to add a wrapper:
>
> int serial8250_em485_init_by_line(int line) {
>     struct uart_8250_port *uart = &serial8250_ports[line];
>     int ret;
>     unsigned long flags;
>
>     spin_lock_irqsave(&uart->port.lock, flags);
>     ret = serial8250_em485_init(uart).
>     spin_unlock_irqrestore(&uart->port.lock, flags);
>
>     return ret;
> }

Also, I noticed that in 8250_core.c the em485 tx handlers change the
MCR register itself. But in the case when RTS signal is itself
emulated, say by flipping a GPIO, a more generic approach would be
necessary, such as get_mctrl/set_mctrl, so that the use of helpers in
serial_mctrl_gpio.c is straightforward.

For example, in our design the RS232<->RS485 direction switch is
driven by a pin which cannot be pinmuxed as UART RTS but can be a
GPIO. Since it is already in production, making incompatible changes
to the design (and getting away with it) is not inherently feasible.


Regards,
Ilyas G.

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

[Matwey V. Kornilov]

2016-02-11 23:19 GMT+03:00 Ильяс Гасанов <torso.nafi@gmail.com>:
> 2016-02-11 22:08 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
>> Thanks for pointing out. serial8250_unregister_port should set
>> serial8250_ports[line].em485 to NULL in order to prevent unneeded
>> activation when this struct is reused.
>
> Then the allocated/initialized resources should be freed/released as well.

Sure, I've already sketched the patch, but to test it, I need some
time to build 8250_omap as a module and setup networking console and
kgdb over network.

>
>> You would be able to use serial8250_em485_register in omap8250_probe
>> on &up before serial8250_register_8250_port(&up) if
>> serial8250_register_8250_port could replicate em485 member state.
>> But there are alternatives in implementation.
>
> I'm considering adding the relevant code to the omap8250_startup and
> omap8250_shutdown callback functions. However the serial driver
> documentation claims that these don't have port->lock taken when
> invoked, only port_sem.
>
>> serial8250_register_8250_port may either copy pointer up->em485 to
>> uart->em485 or perform deep copy.
>
> Actually, "up" here in omap8250_probe is not a pointer but a struct
> variable on stack, so copying the pointer to it is out of question.
>

up->em485 is always pointer, however you are right, that
serial8250_em485_init stores pointer to up when the timers are inited.
More complex issue here that serial8250_em485_init need to set RTS to
proper state and probably can't do that before
serial8250_register_8250_port. So omap8250_probe should directly or
indirectly use serial8250_get_port(priv->line) after
serial8250_register_8250_port. As for me, possible solution could be
to add a wrapper:

int serial8250_em485_init_by_line(int line) {
    struct uart_8250_port *uart = &serial8250_ports[line];
    int ret;
    unsigned long flags;

    spin_lock_irqsave(&uart->port.lock, flags);
    ret = serial8250_em485_init(uart).
    spin_unlock_irqrestore(&uart->port.lock, flags);

    return ret;
}

>
> Regards,
> Ilyas G.
>



-- 
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 v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Ильяс Гасанов]

2016-02-11 22:08 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
> Thanks for pointing out. serial8250_unregister_port should set
> serial8250_ports[line].em485 to NULL in order to prevent unneeded
> activation when this struct is reused.

Then the allocated/initialized resources should be freed/released as well.

> You would be able to use serial8250_em485_register in omap8250_probe
> on &up before serial8250_register_8250_port(&up) if
> serial8250_register_8250_port could replicate em485 member state.
> But there are alternatives in implementation.

I'm considering adding the relevant code to the omap8250_startup and
omap8250_shutdown callback functions. However the serial driver
documentation claims that these don't have port->lock taken when
invoked, only port_sem.

> serial8250_register_8250_port may either copy pointer up->em485 to
> uart->em485 or perform deep copy.

Actually, "up" here in omap8250_probe is not a pointer but a struct
variable on stack, so copying the pointer to it is out of question.


Regards,
Ilyas G.

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

[Matwey V. Kornilov]

2016-02-11 19:40 GMT+03:00 Ильяс Гасанов <torso.nafi@gmail.com>:
> Hello,
>
> 2016-02-01 21:09 GMT+03:00 "Matwey V. Kornilov" <matwey@sai.msu.ru>:
>> +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_init and serial8250_em485_destroy
>> +        * are idempotent
>> +        */
>> +       if (rs485->flags & SER_RS485_ENABLED) {
>> +               int ret = serial8250_em485_init(up);
>> +
>> +               if (ret) {
>> +                       rs485->flags &= ~SER_RS485_ENABLED;
>> +                       port->rs485.flags &= ~SER_RS485_ENABLED;
>> +               }
>> +               return ret;
>> +       }
>> +
>> +       serial8250_em485_destroy(up);
>> +
>> +       return 0;
>> +}
>
> I think that an invocation of serial8250_em485_destroy() should be
> performed inside omap8250_remove() as well. However there is a catch -
> serial8250_get_port() is only reserved for suspend-resume power
> management operations, and I am aware of no other means how to supply
> an uart_8250_port to serial8250_em485_destroy() at that point.

Thanks for pointing out. serial8250_unregister_port should set
serial8250_ports[line].em485 to NULL in order to prevent unneeded
activation when this struct is reused.

>
> Similarly, I got no idea on how to implement handling
> "linux,rs485-enabled-at-boot-time" property without using
> serial8250_get_port() directly (well, honestly I could try invoking
> the corresponding ioctl somehow, but that's gonna be plain ugly at
> least).

You would be able to use serial8250_em485_register in omap8250_probe
on &up before serial8250_register_8250_port(&up) if
serial8250_register_8250_port could replicate em485 member state.
But there are alternatives in implementation.
serial8250_register_8250_port may either copy pointer up->em485 to
uart->em485 or perform deep copy.

>
> Regards,
> Ilyas G.
>



-- 
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 v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Ильяс Гасанов]

Hello,

2016-02-01 21:09 GMT+03:00 "Matwey V. Kornilov" <matwey@sai.msu.ru>:
> +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_init and serial8250_em485_destroy
> +        * are idempotent
> +        */
> +       if (rs485->flags & SER_RS485_ENABLED) {
> +               int ret = serial8250_em485_init(up);
> +
> +               if (ret) {
> +                       rs485->flags &= ~SER_RS485_ENABLED;
> +                       port->rs485.flags &= ~SER_RS485_ENABLED;
> +               }
> +               return ret;
> +       }
> +
> +       serial8250_em485_destroy(up);
> +
> +       return 0;
> +}

I think that an invocation of serial8250_em485_destroy() should be
performed inside omap8250_remove() as well. However there is a catch -
serial8250_get_port() is only reserved for suspend-resume power
management operations, and I am aware of no other means how to supply
an uart_8250_port to serial8250_em485_destroy() at that point.

Similarly, I got no idea on how to implement handling
"linux,rs485-enabled-at-boot-time" property without using
serial8250_get_port() directly (well, honestly I could try invoking
the corresponding ioctl somehow, but that's gonna be plain ugly at
least).

Regards,
Ilyas G.