* [PATCH v6 0/3] tty: Introduce software RS485 direction control support
@ 2015-12-21 18:26 Matwey V. Kornilov
2015-12-21 18:26 ` [PATCH v6 1/3] tty: Move serial8250_stop_rx in front of serial8250_start_tx Matwey V. Kornilov
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Matwey V. Kornilov @ 2015-12-21 18:26 UTC (permalink / raw)
To: gregkh, jslaby, peter, andy.shevchenko, gnomes
Cc: Matwey V. Kornilov, linux-kernel, linux-serial
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 1/3] tty: Move serial8250_stop_rx in front of serial8250_start_tx
2015-12-21 18:26 [PATCH v6 0/3] tty: Introduce software RS485 direction control support Matwey V. Kornilov
@ 2015-12-21 18:26 ` Matwey V. Kornilov
2015-12-21 18:26 ` [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 Matwey V. Kornilov
2015-12-21 18:26 ` [PATCH v6 3/3] tty: 8250_omap: Use software emulated RS485 direction control Matwey V. Kornilov
2 siblings, 0 replies; 17+ messages in thread
From: Matwey V. Kornilov @ 2015-12-21 18:26 UTC (permalink / raw)
To: gregkh, jslaby, peter, andy.shevchenko, gnomes
Cc: Matwey V. Kornilov, linux-kernel, linux-serial
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 0bbf340..8ad0b2d 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1280,6 +1280,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) {
@@ -1347,19 +1360,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.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
2015-12-21 18:26 [PATCH v6 0/3] tty: Introduce software RS485 direction control support Matwey V. Kornilov
2015-12-21 18:26 ` [PATCH v6 1/3] tty: Move serial8250_stop_rx in front of serial8250_start_tx Matwey V. Kornilov
@ 2015-12-21 18:26 ` Matwey V. Kornilov
2016-01-15 16:14 ` Peter Hurley
2015-12-21 18:26 ` [PATCH v6 3/3] tty: 8250_omap: Use software emulated RS485 direction control Matwey V. Kornilov
2 siblings, 1 reply; 17+ messages in thread
From: Matwey V. Kornilov @ 2015-12-21 18:26 UTC (permalink / raw)
To: gregkh, jslaby, peter, andy.shevchenko, gnomes
Cc: Matwey V. Kornilov, linux-kernel, linux-serial
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 | 6 ++
drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++-
include/linux/serial_8250.h | 7 ++
3 files changed, 170 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index d54dcd8..0189cb3 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -117,6 +117,12 @@ 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);
+static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
+{
+ return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
+}
#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 8ad0b2d..d67a848 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>
@@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
}
}
+static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
+{
+ unsigned char mcr = serial_in(p, UART_MCR);
+
+ if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
+ mcr |= UART_MCR_RTS;
+ else
+ mcr &= ~UART_MCR_RTS;
+ serial_out(p, UART_MCR, mcr);
+}
+
+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);
@@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
}
EXPORT_SYMBOL_GPL(serial8250_rpm_put);
+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;
+
+ init_timer(&p->em485->stop_tx_timer);
+ p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
+ p->em485->stop_tx_timer.data = (unsigned long)p;
+ p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
+ init_timer(&p->em485->start_tx_timer);
+ p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
+ p->em485->start_tx_timer.data = (unsigned long)p;
+ p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
+
+ serial8250_em485_rts_after_send(p);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(serial8250_em485_init);
+void serial8250_em485_destroy(struct uart_8250_port *p)
+{
+ if (p->em485 == NULL)
+ return;
+
+ del_timer_sync(&p->em485->start_tx_timer);
+ del_timer_sync(&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
@@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
serial8250_rpm_put(up);
}
-static inline void __stop_tx(struct uart_8250_port *p)
+static __u32 __start_tx_rs485(struct uart_8250_port *p)
+{
+ if (!serial8250_em485_enabled(p))
+ return 0;
+
+ if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
+ serial8250_stop_rx(&p->port);
+
+ del_timer_sync(&p->em485->stop_tx_timer);
+
+ if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
+ serial8250_em485_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)
+{
+ if (!serial8250_em485_enabled(p))
+ 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;
+
+ __do_stop_tx_rs485(p);
+}
+
+static inline void __stop_tx_rs485(struct uart_8250_port *p)
+{
+ if (!serial8250_em485_enabled(p))
+ return;
+
+ del_timer_sync(&p->em485->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->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;
@@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p)
}
}
+static inline void __stop_tx(struct uart_8250_port *p)
+{
+ if (serial8250_em485_enabled(p)) {
+ 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 & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
+ return;
+ }
+ __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);
@@ -1319,12 +1450,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;
@@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port)
}
}
+static void serial8250_em485_handle_start_tx(unsigned long arg)
+{
+ struct uart_8250_port *p = (struct uart_8250_port *)arg;
+
+ __start_tx(&p->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 (up->em485 && timer_pending(&up->em485->start_tx_timer))
+ return;
+
+ if (up->em485 && (delay = __start_tx_rs485(up))) {
+ mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
+ } 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..71516ec 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -76,6 +76,11 @@ 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 */
+};
+
/*
* This should be used by drivers which want to register
* their own 8250 ports without registering their own
@@ -122,6 +127,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.6.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v6 3/3] tty: 8250_omap: Use software emulated RS485 direction control
2015-12-21 18:26 [PATCH v6 0/3] tty: Introduce software RS485 direction control support Matwey V. Kornilov
2015-12-21 18:26 ` [PATCH v6 1/3] tty: Move serial8250_stop_rx in front of serial8250_start_tx Matwey V. Kornilov
2015-12-21 18:26 ` [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 Matwey V. Kornilov
@ 2015-12-21 18:26 ` Matwey V. Kornilov
2016-01-15 16:32 ` Peter Hurley
2 siblings, 1 reply; 17+ messages in thread
From: Matwey V. Kornilov @ 2015-12-21 18:26 UTC (permalink / raw)
To: gregkh, jslaby, peter, andy.shevchenko, gnomes
Cc: Matwey V. Kornilov, linux-kernel, linux-serial
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 | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 826c5c4..323c0a4 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -698,6 +698,23 @@ 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);
+
+ if (rs485->flags & SER_RS485_ENABLED && !serial8250_em485_enabled(up)) {
+ port->rs485 = *rs485;
+ return serial8250_em485_init(up);
+ }
+
+ if (serial8250_em485_enabled(up) && !(rs485->flags & SER_RS485_ENABLED))
+ serial8250_em485_destroy(up);
+
+ port->rs485 = *rs485;
+
+ return 0;
+}
+
static void omap_8250_unthrottle(struct uart_port *port)
{
unsigned long flags;
@@ -1144,6 +1161,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.6.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
2015-12-21 18:26 ` [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 Matwey V. Kornilov
@ 2016-01-15 16:14 ` Peter Hurley
2016-01-15 16:40 ` Peter Hurley
2016-01-15 18:42 ` Matwey V. Kornilov
0 siblings, 2 replies; 17+ messages in thread
From: Peter Hurley @ 2016-01-15 16:14 UTC (permalink / raw)
To: Matwey V. Kornilov
Cc: gregkh, jslaby, andy.shevchenko, gnomes, linux-kernel, linux-serial
On 12/21/2015 10:26 AM, Matwey V. Kornilov 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.
Apologies for the long delay; comments below.
> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> ---
> drivers/tty/serial/8250/8250.h | 6 ++
> drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++-
> include/linux/serial_8250.h | 7 ++
> 3 files changed, 170 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index d54dcd8..0189cb3 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -117,6 +117,12 @@ 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);
> +static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
> +{
> + return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
Under what circumstances is p->em485 != NULL but
(p->port.rs485.flags & SER_RS485_ENABLED) is true?
ISTM, p->em485 is necessary and sufficient to determine if em485 is enabled.
In which case, this function can be eliminated and callers can be reduced to
if (p->em485)
....
> +}
>
> #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 8ad0b2d..d67a848 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>
> @@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
> }
> }
>
> +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
Only one call site, so please drop inline.
> +{
> + unsigned char mcr = serial_in(p, UART_MCR);
> +
> + if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
> + mcr |= UART_MCR_RTS;
> + else
> + mcr &= ~UART_MCR_RTS;
> + serial_out(p, UART_MCR, mcr);
> +}
> +
> +static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
Doesn't really need to be inline.
> +{
> + 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);
> @@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
> }
> EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>
> +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;
> +
> + init_timer(&p->em485->stop_tx_timer);
> + p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
> + p->em485->stop_tx_timer.data = (unsigned long)p;
> + p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
Not sure this is going to fly; this would be the only user of TIMER_IRQSAFE
(which was specifically introduced to workaround workqueue issues and not
meant for general use).
> + init_timer(&p->em485->start_tx_timer);
> + p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
> + p->em485->start_tx_timer.data = (unsigned long)p;
> + p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
> +
> + serial8250_em485_rts_after_send(p);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(serial8250_em485_init);
Newline.
> +void serial8250_em485_destroy(struct uart_8250_port *p)
> +{
> + if (p->em485 == NULL)
> + return;
> +
> + del_timer_sync(&p->em485->start_tx_timer);
> + del_timer_sync(&p->em485->stop_tx_timer);
What keeps start_tx() from restarting a new timer right here?
> + 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
> @@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
> serial8250_rpm_put(up);
> }
>
> -static inline void __stop_tx(struct uart_8250_port *p)
> +static __u32 __start_tx_rs485(struct uart_8250_port *p)
^^^^^
No need to preserve the userspace type here.
The double underline leader in an identifier is typically used to distinguish
an unlocked version from a locked version. I don't think it's necessary here
or any of the other newly-introduced functions.
> +{
> + if (!serial8250_em485_enabled(p))
> + return 0;
Already checked that em485 was enabled in lone caller.
> + if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
> + serial8250_stop_rx(&p->port);
> +
> + del_timer_sync(&p->em485->stop_tx_timer);
> +
> + if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
Line too long. And just one negation is sufficient, ie.
if (!(....) !=
!(....)) {
> + serial8250_em485_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)
Does this really need to be inline?
> +{
> + if (!serial8250_em485_enabled(p))
> + return;
> +
> + serial8250_em485_rts_after_send(p);
> + /*
> + * Empty the RX FIFO, we are not interested in anything
> + * received during the half-duplex transmission.
> + */
Malformed block comment.
/*
*
*
*/
> + 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;
> +
> + __do_stop_tx_rs485(p);
> +}
> +
> +static inline void __stop_tx_rs485(struct uart_8250_port *p)
Single caller so drop inline.
> +{
> + if (!serial8250_em485_enabled(p))
> + return;
> +
> + del_timer_sync(&p->em485->start_tx_timer);
> +
> + /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
Block comment.
> + if (p->port.rs485.delay_rts_after_send > 0) {
> + mod_timer(&p->em485->stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
Line too long; please re-format.
This is one problem with overly long identifiers.
> + } 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;
> @@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p)
> }
> }
>
> +static inline void __stop_tx(struct uart_8250_port *p)
> +{
> + if (serial8250_em485_enabled(p)) {
> + 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.
> + */
Block indent.
This code path should cancel start timer also.
> + if (!((lsr & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
> + return;
> + }
> + __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);
> @@ -1319,12 +1450,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;
>
> @@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port)
> }
> }
>
> +static void serial8250_em485_handle_start_tx(unsigned long arg)
> +{
> + struct uart_8250_port *p = (struct uart_8250_port *)arg;
> +
> + __start_tx(&p->port);
> +}
> +
> +static void serial8250_start_tx(struct uart_port *port)
> +{
> + struct uart_8250_port *up = up_to_u8250p(port);
> + __u32 delay;
int delay;
> +
> + serial8250_rpm_get_tx(up);
> +
> + if (up->em485 && timer_pending(&up->em485->start_tx_timer))
> + return;
> +
> + if (up->em485 && (delay = __start_tx_rs485(up))) {
No assignment in conditional please.
> + mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
> + } else {
> + __start_tx(port);
> + }
Generally, braces aren't used for single statement if..else.
That probably won't apply here after removing the assignment-in-conditional,
but I thought it worth mentioning just so you know.
Regards,
Peter Hurley
> +}
> +
> 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..71516ec 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -76,6 +76,11 @@ 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 */
> +};
> +
> /*
> * This should be used by drivers which want to register
> * their own 8250 ports without registering their own
> @@ -122,6 +127,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)
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 3/3] tty: 8250_omap: Use software emulated RS485 direction control
2015-12-21 18:26 ` [PATCH v6 3/3] tty: 8250_omap: Use software emulated RS485 direction control Matwey V. Kornilov
@ 2016-01-15 16:32 ` Peter Hurley
0 siblings, 0 replies; 17+ messages in thread
From: Peter Hurley @ 2016-01-15 16:32 UTC (permalink / raw)
To: Matwey V. Kornilov
Cc: gregkh, jslaby, andy.shevchenko, gnomes, linux-kernel, linux-serial
On 12/21/2015 10:26 AM, Matwey V. Kornilov 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.
Again, sorry for the long delay; comments below.
> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> ---
> drivers/tty/serial/8250/8250_omap.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 826c5c4..323c0a4 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -698,6 +698,23 @@ 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);
> +
> + if (rs485->flags & SER_RS485_ENABLED && !serial8250_em485_enabled(up)) {
> + port->rs485 = *rs485;
Please clamp the delay values to something reasonable; 100ms?
> + return serial8250_em485_init(up);
> + }
> +
> + if (serial8250_em485_enabled(up) && !(rs485->flags & SER_RS485_ENABLED))
> + serial8250_em485_destroy(up);
> +
> + port->rs485 = *rs485;
This logic seems a little convoluted; you're checking the state here but
then you're re-checking the state in init/destroy.
Regards,
Peter Hurley
> +
> + return 0;
> +}
> +
> static void omap_8250_unthrottle(struct uart_port *port)
> {
> unsigned long flags;
> @@ -1144,6 +1161,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;
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
2016-01-15 16:14 ` Peter Hurley
@ 2016-01-15 16:40 ` Peter Hurley
2016-01-15 18:42 ` Matwey V. Kornilov
1 sibling, 0 replies; 17+ messages in thread
From: Peter Hurley @ 2016-01-15 16:40 UTC (permalink / raw)
To: Matwey V. Kornilov
Cc: gregkh, jslaby, andy.shevchenko, gnomes, linux-kernel, linux-serial
On 01/15/2016 08:14 AM, Peter Hurley wrote:
> On 12/21/2015 10:26 AM, Matwey V. Kornilov 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.
>
> Apologies for the long delay; comments below.
More comments below.
>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>> ---
>> drivers/tty/serial/8250/8250.h | 6 ++
>> drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++-
>> include/linux/serial_8250.h | 7 ++
>> 3 files changed, 170 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>> index d54dcd8..0189cb3 100644
>> --- a/drivers/tty/serial/8250/8250.h
>> +++ b/drivers/tty/serial/8250/8250.h
>> @@ -117,6 +117,12 @@ 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);
>> +static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
>> +{
>> + return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
>
> Under what circumstances is p->em485 != NULL but
> (p->port.rs485.flags & SER_RS485_ENABLED) is true?
>
> ISTM, p->em485 is necessary and sufficient to determine if em485 is enabled.
>
> In which case, this function can be eliminated and callers can be reduced to
>
> if (p->em485)
> ....
>
>> +}
>>
>> #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 8ad0b2d..d67a848 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>
>> @@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>> }
>> }
>>
>> +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
>
> Only one call site, so please drop inline.
>
>
>> +{
>> + unsigned char mcr = serial_in(p, UART_MCR);
>> +
>> + if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>> + mcr |= UART_MCR_RTS;
>> + else
>> + mcr &= ~UART_MCR_RTS;
>> + serial_out(p, UART_MCR, mcr);
>> +}
>> +
>> +static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
>
> Doesn't really need to be inline.
>
>
>> +{
>> + 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);
>> @@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
>> }
>> EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>
>> +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;
>> +
>> + init_timer(&p->em485->stop_tx_timer);
>> + p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
>> + p->em485->stop_tx_timer.data = (unsigned long)p;
>> + p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
>
> Not sure this is going to fly; this would be the only user of TIMER_IRQSAFE
> (which was specifically introduced to workaround workqueue issues and not
> meant for general use).
>
>
>> + init_timer(&p->em485->start_tx_timer);
>> + p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
>> + p->em485->start_tx_timer.data = (unsigned long)p;
>> + p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
>> +
>> + serial8250_em485_rts_after_send(p);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(serial8250_em485_init);
>
> Newline.
>
>> +void serial8250_em485_destroy(struct uart_8250_port *p)
>> +{
>> + if (p->em485 == NULL)
>> + return;
>> +
>> + del_timer_sync(&p->em485->start_tx_timer);
>> + del_timer_sync(&p->em485->stop_tx_timer);
>
> What keeps start_tx() from restarting a new timer right here?
>
>> + 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
>> @@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
>> serial8250_rpm_put(up);
>> }
>>
>> -static inline void __stop_tx(struct uart_8250_port *p)
>> +static __u32 __start_tx_rs485(struct uart_8250_port *p)
> ^^^^^
> No need to preserve the userspace type here.
>
> The double underline leader in an identifier is typically used to distinguish
> an unlocked version from a locked version. I don't think it's necessary here
> or any of the other newly-introduced functions.
>
>
>> +{
>> + if (!serial8250_em485_enabled(p))
>> + return 0;
>
> Already checked that em485 was enabled in lone caller.
>
>
>> + if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>> + serial8250_stop_rx(&p->port);
>> +
>> + del_timer_sync(&p->em485->stop_tx_timer);
>> +
>> + if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
>
> Line too long. And just one negation is sufficient, ie.
>
> if (!(....) !=
> !(....)) {
>
>> + serial8250_em485_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)
>
> Does this really need to be inline?
>
>> +{
>> + if (!serial8250_em485_enabled(p))
>> + return;
>> +
>> + serial8250_em485_rts_after_send(p);
>> + /*
>> + * Empty the RX FIFO, we are not interested in anything
>> + * received during the half-duplex transmission.
>> + */
>
> Malformed block comment.
>
> /*
> *
> *
> */
>
>> + 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;
>> +
>> + __do_stop_tx_rs485(p);
>> +}
>> +
>> +static inline void __stop_tx_rs485(struct uart_8250_port *p)
>
> Single caller so drop inline.
>
>> +{
>> + if (!serial8250_em485_enabled(p))
>> + return;
>> +
>> + del_timer_sync(&p->em485->start_tx_timer);
>> +
>> + /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
>
> Block comment.
>
>> + if (p->port.rs485.delay_rts_after_send > 0) {
>> + mod_timer(&p->em485->stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
>
> Line too long; please re-format.
> This is one problem with overly long identifiers.
>
>> + } 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;
>> @@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p)
>> }
>> }
>>
>> +static inline void __stop_tx(struct uart_8250_port *p)
>> +{
>> + if (serial8250_em485_enabled(p)) {
>> + 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.
>> + */
>
> Block indent.
>
> This code path should cancel start timer also.
>
>> + if (!((lsr & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
>
> if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>
>> + return;
>> + }
>> + __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);
>> @@ -1319,12 +1450,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;
>>
>> @@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port)
>> }
>> }
>>
>> +static void serial8250_em485_handle_start_tx(unsigned long arg)
>> +{
>> + struct uart_8250_port *p = (struct uart_8250_port *)arg;
>> +
>> + __start_tx(&p->port);
>> +}
>> +
>> +static void serial8250_start_tx(struct uart_port *port)
>> +{
>> + struct uart_8250_port *up = up_to_u8250p(port);
>> + __u32 delay;
>
> int delay;
Actually, just refactor the mod_timer() below into __start_tx_rs485();
'delay' and assignment-in-conditional goes away.
>> +
>> + serial8250_rpm_get_tx(up);
>> +
>> + if (up->em485 && timer_pending(&up->em485->start_tx_timer))
>> + return;
>> +
>> + if (up->em485 && (delay = __start_tx_rs485(up))) {
>
> No assignment in conditional please.
>
>> + mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
>> + } else {
>> + __start_tx(port);
>> + }
>
> Generally, braces aren't used for single statement if..else.
> That probably won't apply here after removing the assignment-in-conditional,
> but I thought it worth mentioning just so you know.
>
> Regards,
> Peter Hurley
>
>> +}
>> +
>> 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..71516ec 100644
>> --- a/include/linux/serial_8250.h
>> +++ b/include/linux/serial_8250.h
>> @@ -76,6 +76,11 @@ 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 */
>> +};
>> +
>> /*
>> * This should be used by drivers which want to register
>> * their own 8250 ports without registering their own
>> @@ -122,6 +127,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)
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
2016-01-15 16:14 ` Peter Hurley
2016-01-15 16:40 ` Peter Hurley
@ 2016-01-15 18:42 ` Matwey V. Kornilov
2016-01-15 19:45 ` Peter Hurley
1 sibling, 1 reply; 17+ messages in thread
From: Matwey V. Kornilov @ 2016-01-15 18:42 UTC (permalink / raw)
To: Peter Hurley
Cc: Greg KH, Jiri Slaby, Andy Shevchenko, One Thousand Gnomes,
linux-kernel, linux-serial
2016-01-15 19:14 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> On 12/21/2015 10:26 AM, Matwey V. Kornilov 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.
>
> Apologies for the long delay; comments below.
>
>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>> ---
>> drivers/tty/serial/8250/8250.h | 6 ++
>> drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++-
>> include/linux/serial_8250.h | 7 ++
>> 3 files changed, 170 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>> index d54dcd8..0189cb3 100644
>> --- a/drivers/tty/serial/8250/8250.h
>> +++ b/drivers/tty/serial/8250/8250.h
>> @@ -117,6 +117,12 @@ 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);
>> +static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
>> +{
>> + return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
>
> Under what circumstances is p->em485 != NULL but
> (p->port.rs485.flags & SER_RS485_ENABLED) is true?
>
> ISTM, p->em485 is necessary and sufficient to determine if em485 is enabled.
>
> In which case, this function can be eliminated and callers can be reduced to
>
> if (p->em485)
> ....
>
>> +}
>>
>> #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 8ad0b2d..d67a848 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>
>> @@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>> }
>> }
>>
>> +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
>
> Only one call site, so please drop inline.
>
>
>> +{
>> + unsigned char mcr = serial_in(p, UART_MCR);
>> +
>> + if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>> + mcr |= UART_MCR_RTS;
>> + else
>> + mcr &= ~UART_MCR_RTS;
>> + serial_out(p, UART_MCR, mcr);
>> +}
>> +
>> +static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
>
> Doesn't really need to be inline.
>
>
>> +{
>> + 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);
>> @@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
>> }
>> EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>
>> +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;
>> +
>> + init_timer(&p->em485->stop_tx_timer);
>> + p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
>> + p->em485->stop_tx_timer.data = (unsigned long)p;
>> + p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
>
> Not sure this is going to fly; this would be the only user of TIMER_IRQSAFE
> (which was specifically introduced to workaround workqueue issues and not
> meant for general use).
This is required to call del_timer_sync(&p->em485->start_tx_timer);
from __stop_tx_rs485
>
>
>> + init_timer(&p->em485->start_tx_timer);
>> + p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
>> + p->em485->start_tx_timer.data = (unsigned long)p;
>> + p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
>> +
>> + serial8250_em485_rts_after_send(p);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(serial8250_em485_init);
>
> Newline.
>
>> +void serial8250_em485_destroy(struct uart_8250_port *p)
>> +{
>> + if (p->em485 == NULL)
>> + return;
>> +
>> + del_timer_sync(&p->em485->start_tx_timer);
>> + del_timer_sync(&p->em485->stop_tx_timer);
>
> What keeps start_tx() from restarting a new timer right here?
Both start_tx and rs485_config (which calls destroy) are wrapped with
port->lock in serial_core.c
>
>> + 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
>> @@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
>> serial8250_rpm_put(up);
>> }
>>
>> -static inline void __stop_tx(struct uart_8250_port *p)
>> +static __u32 __start_tx_rs485(struct uart_8250_port *p)
> ^^^^^
> No need to preserve the userspace type here.
>
> The double underline leader in an identifier is typically used to distinguish
> an unlocked version from a locked version. I don't think it's necessary here
> or any of the other newly-introduced functions.
I use double __ for consistency with __start_tx. Now I have:
if (up->em485)
__start_tx_rs485(port);
else
__start_tx(port);
>
>
>> +{
>> + if (!serial8250_em485_enabled(p))
>> + return 0;
>
> Already checked that em485 was enabled in lone caller.
>
>
>> + if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>> + serial8250_stop_rx(&p->port);
>> +
>> + del_timer_sync(&p->em485->stop_tx_timer);
>> +
>> + if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
>
> Line too long. And just one negation is sufficient, ie.
>
> if (!(....) !=
> !(....)) {
>
I would like to keep the double negation, in my opinion it is more
clear to the reader and I believe that the compiler is able to
optimize it.
>> + serial8250_em485_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)
>
> Does this really need to be inline?
>
Why not?
>> +{
>> + if (!serial8250_em485_enabled(p))
>> + return;
>> +
>> + serial8250_em485_rts_after_send(p);
>> + /*
>> + * Empty the RX FIFO, we are not interested in anything
>> + * received during the half-duplex transmission.
>> + */
>
> Malformed block comment.
>
> /*
> *
> *
> */
>
>> + 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;
>> +
>> + __do_stop_tx_rs485(p);
>> +}
>> +
>> +static inline void __stop_tx_rs485(struct uart_8250_port *p)
>
> Single caller so drop inline.
>
>> +{
>> + if (!serial8250_em485_enabled(p))
>> + return;
>> +
>> + del_timer_sync(&p->em485->start_tx_timer);
>> +
>> + /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
>
> Block comment.
>
>> + if (p->port.rs485.delay_rts_after_send > 0) {
>> + mod_timer(&p->em485->stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
>
> Line too long; please re-format.
> This is one problem with overly long identifiers.
>
>> + } 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;
>> @@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p)
>> }
>> }
>>
>> +static inline void __stop_tx(struct uart_8250_port *p)
>> +{
>> + if (serial8250_em485_enabled(p)) {
>> + 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.
>> + */
>
> Block indent.
>
> This code path should cancel start timer also.
>
>> + if (!((lsr & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
>
> if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>
>> + return;
>> + }
>> + __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);
>> @@ -1319,12 +1450,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;
>>
>> @@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port)
>> }
>> }
>>
>> +static void serial8250_em485_handle_start_tx(unsigned long arg)
>> +{
>> + struct uart_8250_port *p = (struct uart_8250_port *)arg;
>> +
>> + __start_tx(&p->port);
>> +}
>> +
>> +static void serial8250_start_tx(struct uart_port *port)
>> +{
>> + struct uart_8250_port *up = up_to_u8250p(port);
>> + __u32 delay;
>
> int delay;
>
>> +
>> + serial8250_rpm_get_tx(up);
>> +
>> + if (up->em485 && timer_pending(&up->em485->start_tx_timer))
>> + return;
>> +
>> + if (up->em485 && (delay = __start_tx_rs485(up))) {
>
> No assignment in conditional please.
>
>> + mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
>> + } else {
>> + __start_tx(port);
>> + }
>
> Generally, braces aren't used for single statement if..else.
> That probably won't apply here after removing the assignment-in-conditional,
> but I thought it worth mentioning just so you know.
>
> Regards,
> Peter Hurley
>
>> +}
>> +
>> 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..71516ec 100644
>> --- a/include/linux/serial_8250.h
>> +++ b/include/linux/serial_8250.h
>> @@ -76,6 +76,11 @@ 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 */
>> +};
>> +
>> /*
>> * This should be used by drivers which want to register
>> * their own 8250 ports without registering their own
>> @@ -122,6 +127,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)
>>
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
2016-01-15 18:42 ` Matwey V. Kornilov
@ 2016-01-15 19:45 ` Peter Hurley
2016-01-15 20:01 ` Matwey V. Kornilov
2016-01-15 20:20 ` Matwey V. Kornilov
0 siblings, 2 replies; 17+ messages in thread
From: Peter Hurley @ 2016-01-15 19:45 UTC (permalink / raw)
To: Matwey V. Kornilov
Cc: Greg KH, Jiri Slaby, Andy Shevchenko, One Thousand Gnomes,
linux-kernel, linux-serial
On 01/15/2016 10:42 AM, Matwey V. Kornilov wrote:
> 2016-01-15 19:14 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>> On 12/21/2015 10:26 AM, Matwey V. Kornilov 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.
>>
>> Apologies for the long delay; comments below.
>>
>>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>>> ---
>>> drivers/tty/serial/8250/8250.h | 6 ++
>>> drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++-
>>> include/linux/serial_8250.h | 7 ++
>>> 3 files changed, 170 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>>> index d54dcd8..0189cb3 100644
>>> --- a/drivers/tty/serial/8250/8250.h
>>> +++ b/drivers/tty/serial/8250/8250.h
>>> @@ -117,6 +117,12 @@ 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);
>>> +static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
>>> +{
>>> + return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
>>
>> Under what circumstances is p->em485 != NULL but
>> (p->port.rs485.flags & SER_RS485_ENABLED) is true?
>>
>> ISTM, p->em485 is necessary and sufficient to determine if em485 is enabled.
>>
>> In which case, this function can be eliminated and callers can be reduced to
>>
>> if (p->em485)
>> ....
>>
>>> +}
>>>
>>> #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 8ad0b2d..d67a848 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>
>>> @@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>>> }
>>> }
>>>
>>> +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
>>
>> Only one call site, so please drop inline.
>>
>>
>>> +{
>>> + unsigned char mcr = serial_in(p, UART_MCR);
>>> +
>>> + if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>>> + mcr |= UART_MCR_RTS;
>>> + else
>>> + mcr &= ~UART_MCR_RTS;
>>> + serial_out(p, UART_MCR, mcr);
>>> +}
>>> +
>>> +static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
>>
>> Doesn't really need to be inline.
>>
>>
>>> +{
>>> + 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);
>>> @@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
>>> }
>>> EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>>
>>> +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;
>>> +
>>> + init_timer(&p->em485->stop_tx_timer);
>>> + p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
>>> + p->em485->stop_tx_timer.data = (unsigned long)p;
>>> + p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
>>
>> Not sure this is going to fly; this would be the only user of TIMER_IRQSAFE
>> (which was specifically introduced to workaround workqueue issues and not
>> meant for general use).
>
> This is required to call del_timer_sync(&p->em485->start_tx_timer);
> from __stop_tx_rs485
I know; that doesn't mean it's ok.
>>> + init_timer(&p->em485->start_tx_timer);
>>> + p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
>>> + p->em485->start_tx_timer.data = (unsigned long)p;
>>> + p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
>>> +
>>> + serial8250_em485_rts_after_send(p);
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(serial8250_em485_init);
>>
>> Newline.
>>
>>> +void serial8250_em485_destroy(struct uart_8250_port *p)
>>> +{
>>> + if (p->em485 == NULL)
>>> + return;
>>> +
>>> + del_timer_sync(&p->em485->start_tx_timer);
>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>
>> What keeps start_tx() from restarting a new timer right here?
>
> Both start_tx and rs485_config (which calls destroy) are wrapped with
> port->lock in serial_core.c
Ahh, missed that.
Maybe it would be better simply to implement the config_rs485()
generically, and just call it from the omap_8250 config_rs485().
And put a note about the locking in a function comment header
/**
* serial8250_config_em485() - rs485 config helper
*
* ....
*/
>>> + 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
>>> @@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
>>> serial8250_rpm_put(up);
>>> }
>>>
>>> -static inline void __stop_tx(struct uart_8250_port *p)
>>> +static __u32 __start_tx_rs485(struct uart_8250_port *p)
>> ^^^^^
>> No need to preserve the userspace type here.
>>
>> The double underline leader in an identifier is typically used to distinguish
>> an unlocked version from a locked version. I don't think it's necessary here
>> or any of the other newly-introduced functions.
>
> I use double __ for consistency with __start_tx. Now I have:
>
> if (up->em485)
> __start_tx_rs485(port);
> else
> __start_tx(port);
But __start_tx() is labelled that way to differentiate it from being identified
as the start_tx() handler (which is serial8250_start_tx()). IOW, contributors
unfamiliar with the 8250 driver itself won't become confused when grepping
for start_tx (or at least the idea is to minimize that confusion).
start_tx_rs485() doesn't need differentiation, so doesn't require the
double __ leader.
FWIW, this is consistent and typical elsewhere in the kernel.
>>> +{
>>> + if (!serial8250_em485_enabled(p))
>>> + return 0;
>>
>> Already checked that em485 was enabled in lone caller.
>>
>>
>>> + if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>>> + serial8250_stop_rx(&p->port);
>>> +
>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>> +
>>> + if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
>>
>> Line too long. And just one negation is sufficient, ie.
>>
>> if (!(....) !=
>> !(....)) {
>>
>
> I would like to keep the double negation, in my opinion it is more
> clear to the reader and I believe that the compiler is able to
> optimize it.
>
>>> + serial8250_em485_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)
>>
>> Does this really need to be inline?
>>
>
> Why not?
The expected yardstick for inline is some demonstrable speed improvement;
otherwise, size is favored.
And __stop_tx() is already inlined in 3 places, which really doesn't
need inlining either -- a call/ret is nothing compared to device i/o.
Regards,
Peter Hurley
>>> +{
>>> + if (!serial8250_em485_enabled(p))
>>> + return;
>>> +
>>> + serial8250_em485_rts_after_send(p);
>>> + /*
>>> + * Empty the RX FIFO, we are not interested in anything
>>> + * received during the half-duplex transmission.
>>> + */
>>
>> Malformed block comment.
>>
>> /*
>> *
>> *
>> */
>>
>>> + 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;
>>> +
>>> + __do_stop_tx_rs485(p);
>>> +}
>>> +
>>> +static inline void __stop_tx_rs485(struct uart_8250_port *p)
>>
>> Single caller so drop inline.
>>
>>> +{
>>> + if (!serial8250_em485_enabled(p))
>>> + return;
>>> +
>>> + del_timer_sync(&p->em485->start_tx_timer);
>>> +
>>> + /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
>>
>> Block comment.
>>
>>> + if (p->port.rs485.delay_rts_after_send > 0) {
>>> + mod_timer(&p->em485->stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
>>
>> Line too long; please re-format.
>> This is one problem with overly long identifiers.
>>
>>> + } 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;
>>> @@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p)
>>> }
>>> }
>>>
>>> +static inline void __stop_tx(struct uart_8250_port *p)
>>> +{
>>> + if (serial8250_em485_enabled(p)) {
>>> + 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.
>>> + */
>>
>> Block indent.
>>
>> This code path should cancel start timer also.
>>
>>> + if (!((lsr & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
>>
>> if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>>
>>> + return;
>>> + }
>>> + __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);
>>> @@ -1319,12 +1450,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;
>>>
>>> @@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port)
>>> }
>>> }
>>>
>>> +static void serial8250_em485_handle_start_tx(unsigned long arg)
>>> +{
>>> + struct uart_8250_port *p = (struct uart_8250_port *)arg;
>>> +
>>> + __start_tx(&p->port);
>>> +}
>>> +
>>> +static void serial8250_start_tx(struct uart_port *port)
>>> +{
>>> + struct uart_8250_port *up = up_to_u8250p(port);
>>> + __u32 delay;
>>
>> int delay;
>>
>>> +
>>> + serial8250_rpm_get_tx(up);
>>> +
>>> + if (up->em485 && timer_pending(&up->em485->start_tx_timer))
>>> + return;
>>> +
>>> + if (up->em485 && (delay = __start_tx_rs485(up))) {
>>
>> No assignment in conditional please.
>>
>>> + mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
>>> + } else {
>>> + __start_tx(port);
>>> + }
>>
>> Generally, braces aren't used for single statement if..else.
>> That probably won't apply here after removing the assignment-in-conditional,
>> but I thought it worth mentioning just so you know.
>>
>> Regards,
>> Peter Hurley
>>
>>> +}
>>> +
>>> 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..71516ec 100644
>>> --- a/include/linux/serial_8250.h
>>> +++ b/include/linux/serial_8250.h
>>> @@ -76,6 +76,11 @@ 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 */
>>> +};
>>> +
>>> /*
>>> * This should be used by drivers which want to register
>>> * their own 8250 ports without registering their own
>>> @@ -122,6 +127,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)
>>>
>>
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
2016-01-15 19:45 ` Peter Hurley
@ 2016-01-15 20:01 ` Matwey V. Kornilov
2016-01-15 21:16 ` Matwey V. Kornilov
2016-01-15 20:20 ` Matwey V. Kornilov
1 sibling, 1 reply; 17+ messages in thread
From: Matwey V. Kornilov @ 2016-01-15 20:01 UTC (permalink / raw)
To: Peter Hurley
Cc: Greg KH, Jiri Slaby, Andy Shevchenko, One Thousand Gnomes,
linux-kernel, linux-serial
2016-01-15 22:45 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> On 01/15/2016 10:42 AM, Matwey V. Kornilov wrote:
>> 2016-01-15 19:14 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>> On 12/21/2015 10:26 AM, Matwey V. Kornilov 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.
>>>
>>> Apologies for the long delay; comments below.
>>>
>>>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>>>> ---
>>>> drivers/tty/serial/8250/8250.h | 6 ++
>>>> drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++-
>>>> include/linux/serial_8250.h | 7 ++
>>>> 3 files changed, 170 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>>>> index d54dcd8..0189cb3 100644
>>>> --- a/drivers/tty/serial/8250/8250.h
>>>> +++ b/drivers/tty/serial/8250/8250.h
>>>> @@ -117,6 +117,12 @@ 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);
>>>> +static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
>>>> +{
>>>> + return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
>>>
>>> Under what circumstances is p->em485 != NULL but
>>> (p->port.rs485.flags & SER_RS485_ENABLED) is true?
>>>
>>> ISTM, p->em485 is necessary and sufficient to determine if em485 is enabled.
>>>
>>> In which case, this function can be eliminated and callers can be reduced to
>>>
>>> if (p->em485)
>>> ....
>>>
>>>> +}
>>>>
>>>> #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 8ad0b2d..d67a848 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>
>>>> @@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>>>> }
>>>> }
>>>>
>>>> +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
>>>
>>> Only one call site, so please drop inline.
>>>
>>>
>>>> +{
>>>> + unsigned char mcr = serial_in(p, UART_MCR);
>>>> +
>>>> + if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>>>> + mcr |= UART_MCR_RTS;
>>>> + else
>>>> + mcr &= ~UART_MCR_RTS;
>>>> + serial_out(p, UART_MCR, mcr);
>>>> +}
>>>> +
>>>> +static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
>>>
>>> Doesn't really need to be inline.
>>>
>>>
>>>> +{
>>>> + 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);
>>>> @@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
>>>> }
>>>> EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>>>
>>>> +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;
>>>> +
>>>> + init_timer(&p->em485->stop_tx_timer);
>>>> + p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
>>>> + p->em485->stop_tx_timer.data = (unsigned long)p;
>>>> + p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
>>>
>>> Not sure this is going to fly; this would be the only user of TIMER_IRQSAFE
>>> (which was specifically introduced to workaround workqueue issues and not
>>> meant for general use).
>>
>> This is required to call del_timer_sync(&p->em485->start_tx_timer);
>> from __stop_tx_rs485
>
> I know; that doesn't mean it's ok.
>
What do you suggest? Run __stop_tx as a tasklet? I am not sure whether
it introduces races or not.
>
>>>> + init_timer(&p->em485->start_tx_timer);
>>>> + p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
>>>> + p->em485->start_tx_timer.data = (unsigned long)p;
>>>> + p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
>>>> +
>>>> + serial8250_em485_rts_after_send(p);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(serial8250_em485_init);
>>>
>>> Newline.
>>>
>>>> +void serial8250_em485_destroy(struct uart_8250_port *p)
>>>> +{
>>>> + if (p->em485 == NULL)
>>>> + return;
>>>> +
>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>
>>> What keeps start_tx() from restarting a new timer right here?
>>
>> Both start_tx and rs485_config (which calls destroy) are wrapped with
>> port->lock in serial_core.c
>
> Ahh, missed that.
>
> Maybe it would be better simply to implement the config_rs485()
> generically, and just call it from the omap_8250 config_rs485().
>
> And put a note about the locking in a function comment header
>
> /**
> * serial8250_config_em485() - rs485 config helper
> *
> * ....
> */
>
>
>
>>>> + 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
>>>> @@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
>>>> serial8250_rpm_put(up);
>>>> }
>>>>
>>>> -static inline void __stop_tx(struct uart_8250_port *p)
>>>> +static __u32 __start_tx_rs485(struct uart_8250_port *p)
>>> ^^^^^
>>> No need to preserve the userspace type here.
>>>
>>> The double underline leader in an identifier is typically used to distinguish
>>> an unlocked version from a locked version. I don't think it's necessary here
>>> or any of the other newly-introduced functions.
>>
>> I use double __ for consistency with __start_tx. Now I have:
>>
>> if (up->em485)
>> __start_tx_rs485(port);
>> else
>> __start_tx(port);
>
> But __start_tx() is labelled that way to differentiate it from being identified
> as the start_tx() handler (which is serial8250_start_tx()). IOW, contributors
> unfamiliar with the 8250 driver itself won't become confused when grepping
> for start_tx (or at least the idea is to minimize that confusion).
>
> start_tx_rs485() doesn't need differentiation, so doesn't require the
> double __ leader.
>
> FWIW, this is consistent and typical elsewhere in the kernel.
>
>
>>>> +{
>>>> + if (!serial8250_em485_enabled(p))
>>>> + return 0;
>>>
>>> Already checked that em485 was enabled in lone caller.
>>>
>>>
>>>> + if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>>>> + serial8250_stop_rx(&p->port);
>>>> +
>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>> +
>>>> + if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
>>>
>>> Line too long. And just one negation is sufficient, ie.
>>>
>>> if (!(....) !=
>>> !(....)) {
>>>
>>
>> I would like to keep the double negation, in my opinion it is more
>> clear to the reader and I believe that the compiler is able to
>> optimize it.
>>
>>>> + serial8250_em485_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)
>>>
>>> Does this really need to be inline?
>>>
>>
>> Why not?
>
> The expected yardstick for inline is some demonstrable speed improvement;
> otherwise, size is favored.
>
> And __stop_tx() is already inlined in 3 places, which really doesn't
> need inlining either -- a call/ret is nothing compared to device i/o.
>
Ok then, probably I am biased with my C++ experience and I am used to
think that compiler considers `inline` only as a hint.
>
> Regards,
> Peter Hurley
>
>>>> +{
>>>> + if (!serial8250_em485_enabled(p))
>>>> + return;
>>>> +
>>>> + serial8250_em485_rts_after_send(p);
>>>> + /*
>>>> + * Empty the RX FIFO, we are not interested in anything
>>>> + * received during the half-duplex transmission.
>>>> + */
>>>
>>> Malformed block comment.
>>>
>>> /*
>>> *
>>> *
>>> */
>>>
>>>> + 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;
>>>> +
>>>> + __do_stop_tx_rs485(p);
>>>> +}
>>>> +
>>>> +static inline void __stop_tx_rs485(struct uart_8250_port *p)
>>>
>>> Single caller so drop inline.
>>>
>>>> +{
>>>> + if (!serial8250_em485_enabled(p))
>>>> + return;
>>>> +
>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>> +
>>>> + /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
>>>
>>> Block comment.
>>>
>>>> + if (p->port.rs485.delay_rts_after_send > 0) {
>>>> + mod_timer(&p->em485->stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
>>>
>>> Line too long; please re-format.
>>> This is one problem with overly long identifiers.
>>>
>>>> + } 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;
>>>> @@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p)
>>>> }
>>>> }
>>>>
>>>> +static inline void __stop_tx(struct uart_8250_port *p)
>>>> +{
>>>> + if (serial8250_em485_enabled(p)) {
>>>> + 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.
>>>> + */
>>>
>>> Block indent.
>>>
>>> This code path should cancel start timer also.
>>>
>>>> + if (!((lsr & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
>>>
>>> if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>>>
>>>> + return;
>>>> + }
>>>> + __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);
>>>> @@ -1319,12 +1450,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;
>>>>
>>>> @@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port)
>>>> }
>>>> }
>>>>
>>>> +static void serial8250_em485_handle_start_tx(unsigned long arg)
>>>> +{
>>>> + struct uart_8250_port *p = (struct uart_8250_port *)arg;
>>>> +
>>>> + __start_tx(&p->port);
>>>> +}
>>>> +
>>>> +static void serial8250_start_tx(struct uart_port *port)
>>>> +{
>>>> + struct uart_8250_port *up = up_to_u8250p(port);
>>>> + __u32 delay;
>>>
>>> int delay;
>>>
>>>> +
>>>> + serial8250_rpm_get_tx(up);
>>>> +
>>>> + if (up->em485 && timer_pending(&up->em485->start_tx_timer))
>>>> + return;
>>>> +
>>>> + if (up->em485 && (delay = __start_tx_rs485(up))) {
>>>
>>> No assignment in conditional please.
>>>
>>>> + mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
>>>> + } else {
>>>> + __start_tx(port);
>>>> + }
>>>
>>> Generally, braces aren't used for single statement if..else.
>>> That probably won't apply here after removing the assignment-in-conditional,
>>> but I thought it worth mentioning just so you know.
>>>
>>> Regards,
>>> Peter Hurley
>>>
>>>> +}
>>>> +
>>>> 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..71516ec 100644
>>>> --- a/include/linux/serial_8250.h
>>>> +++ b/include/linux/serial_8250.h
>>>> @@ -76,6 +76,11 @@ 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 */
>>>> +};
>>>> +
>>>> /*
>>>> * This should be used by drivers which want to register
>>>> * their own 8250 ports without registering their own
>>>> @@ -122,6 +127,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)
>>>>
>>>
>>
>>
>>
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
2016-01-15 19:45 ` Peter Hurley
2016-01-15 20:01 ` Matwey V. Kornilov
@ 2016-01-15 20:20 ` Matwey V. Kornilov
2016-01-15 21:54 ` Peter Hurley
1 sibling, 1 reply; 17+ messages in thread
From: Matwey V. Kornilov @ 2016-01-15 20:20 UTC (permalink / raw)
To: Peter Hurley
Cc: Greg KH, Jiri Slaby, Andy Shevchenko, One Thousand Gnomes,
linux-kernel, linux-serial
2016-01-15 22:45 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> On 01/15/2016 10:42 AM, Matwey V. Kornilov wrote:
>> 2016-01-15 19:14 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>> On 12/21/2015 10:26 AM, Matwey V. Kornilov 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.
>>>
>>> Apologies for the long delay; comments below.
>>>
>>>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>>>> ---
>>>> drivers/tty/serial/8250/8250.h | 6 ++
>>>> drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++-
>>>> include/linux/serial_8250.h | 7 ++
>>>> 3 files changed, 170 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>>>> index d54dcd8..0189cb3 100644
>>>> --- a/drivers/tty/serial/8250/8250.h
>>>> +++ b/drivers/tty/serial/8250/8250.h
>>>> @@ -117,6 +117,12 @@ 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);
>>>> +static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
>>>> +{
>>>> + return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
>>>
>>> Under what circumstances is p->em485 != NULL but
>>> (p->port.rs485.flags & SER_RS485_ENABLED) is true?
>>>
>>> ISTM, p->em485 is necessary and sufficient to determine if em485 is enabled.
>>>
>>> In which case, this function can be eliminated and callers can be reduced to
>>>
>>> if (p->em485)
>>> ....
>>>
>>>> +}
>>>>
>>>> #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 8ad0b2d..d67a848 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>
>>>> @@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>>>> }
>>>> }
>>>>
>>>> +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
>>>
>>> Only one call site, so please drop inline.
>>>
>>>
>>>> +{
>>>> + unsigned char mcr = serial_in(p, UART_MCR);
>>>> +
>>>> + if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>>>> + mcr |= UART_MCR_RTS;
>>>> + else
>>>> + mcr &= ~UART_MCR_RTS;
>>>> + serial_out(p, UART_MCR, mcr);
>>>> +}
>>>> +
>>>> +static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
>>>
>>> Doesn't really need to be inline.
>>>
>>>
>>>> +{
>>>> + 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);
>>>> @@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
>>>> }
>>>> EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>>>
>>>> +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;
>>>> +
>>>> + init_timer(&p->em485->stop_tx_timer);
>>>> + p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
>>>> + p->em485->stop_tx_timer.data = (unsigned long)p;
>>>> + p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
>>>
>>> Not sure this is going to fly; this would be the only user of TIMER_IRQSAFE
>>> (which was specifically introduced to workaround workqueue issues and not
>>> meant for general use).
>>
>> This is required to call del_timer_sync(&p->em485->start_tx_timer);
>> from __stop_tx_rs485
>
> I know; that doesn't mean it's ok.
>
>
>>>> + init_timer(&p->em485->start_tx_timer);
>>>> + p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
>>>> + p->em485->start_tx_timer.data = (unsigned long)p;
>>>> + p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
>>>> +
>>>> + serial8250_em485_rts_after_send(p);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(serial8250_em485_init);
>>>
>>> Newline.
>>>
>>>> +void serial8250_em485_destroy(struct uart_8250_port *p)
>>>> +{
>>>> + if (p->em485 == NULL)
>>>> + return;
>>>> +
>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>
>>> What keeps start_tx() from restarting a new timer right here?
>>
>> Both start_tx and rs485_config (which calls destroy) are wrapped with
>> port->lock in serial_core.c
>
> Ahh, missed that.
>
> Maybe it would be better simply to implement the config_rs485()
> generically, and just call it from the omap_8250 config_rs485().
>
> And put a note about the locking in a function comment header
>
> /**
> * serial8250_config_em485() - rs485 config helper
> *
> * ....
> */
>
I would like to provide init and destroy and left the rest for user.
The reason is simple.
When serial8250_em485_init is called a driver has to enable interrupt
on empty shift register and probably disable it after
serial8250_em485_destroy.
And I believe that the driver knows better how to do it
transactionally and correctly.
The 8250_omap doesn't do that just because it enables the interrupt
normally on start and always keep it enabled.
>
>
>>>> + 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
>>>> @@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
>>>> serial8250_rpm_put(up);
>>>> }
>>>>
>>>> -static inline void __stop_tx(struct uart_8250_port *p)
>>>> +static __u32 __start_tx_rs485(struct uart_8250_port *p)
>>> ^^^^^
>>> No need to preserve the userspace type here.
>>>
>>> The double underline leader in an identifier is typically used to distinguish
>>> an unlocked version from a locked version. I don't think it's necessary here
>>> or any of the other newly-introduced functions.
>>
>> I use double __ for consistency with __start_tx. Now I have:
>>
>> if (up->em485)
>> __start_tx_rs485(port);
>> else
>> __start_tx(port);
>
> But __start_tx() is labelled that way to differentiate it from being identified
> as the start_tx() handler (which is serial8250_start_tx()). IOW, contributors
> unfamiliar with the 8250 driver itself won't become confused when grepping
> for start_tx (or at least the idea is to minimize that confusion).
>
> start_tx_rs485() doesn't need differentiation, so doesn't require the
> double __ leader.
>
> FWIW, this is consistent and typical elsewhere in the kernel.
>
>
>>>> +{
>>>> + if (!serial8250_em485_enabled(p))
>>>> + return 0;
>>>
>>> Already checked that em485 was enabled in lone caller.
>>>
>>>
>>>> + if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>>>> + serial8250_stop_rx(&p->port);
>>>> +
>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>> +
>>>> + if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
>>>
>>> Line too long. And just one negation is sufficient, ie.
>>>
>>> if (!(....) !=
>>> !(....)) {
>>>
>>
>> I would like to keep the double negation, in my opinion it is more
>> clear to the reader and I believe that the compiler is able to
>> optimize it.
>>
>>>> + serial8250_em485_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)
>>>
>>> Does this really need to be inline?
>>>
>>
>> Why not?
>
> The expected yardstick for inline is some demonstrable speed improvement;
> otherwise, size is favored.
>
> And __stop_tx() is already inlined in 3 places, which really doesn't
> need inlining either -- a call/ret is nothing compared to device i/o.
>
>
> Regards,
> Peter Hurley
>
>>>> +{
>>>> + if (!serial8250_em485_enabled(p))
>>>> + return;
>>>> +
>>>> + serial8250_em485_rts_after_send(p);
>>>> + /*
>>>> + * Empty the RX FIFO, we are not interested in anything
>>>> + * received during the half-duplex transmission.
>>>> + */
>>>
>>> Malformed block comment.
>>>
>>> /*
>>> *
>>> *
>>> */
>>>
>>>> + 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;
>>>> +
>>>> + __do_stop_tx_rs485(p);
>>>> +}
>>>> +
>>>> +static inline void __stop_tx_rs485(struct uart_8250_port *p)
>>>
>>> Single caller so drop inline.
>>>
>>>> +{
>>>> + if (!serial8250_em485_enabled(p))
>>>> + return;
>>>> +
>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>> +
>>>> + /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
>>>
>>> Block comment.
>>>
>>>> + if (p->port.rs485.delay_rts_after_send > 0) {
>>>> + mod_timer(&p->em485->stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
>>>
>>> Line too long; please re-format.
>>> This is one problem with overly long identifiers.
>>>
>>>> + } 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;
>>>> @@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p)
>>>> }
>>>> }
>>>>
>>>> +static inline void __stop_tx(struct uart_8250_port *p)
>>>> +{
>>>> + if (serial8250_em485_enabled(p)) {
>>>> + 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.
>>>> + */
>>>
>>> Block indent.
>>>
>>> This code path should cancel start timer also.
>>>
>>>> + if (!((lsr & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
>>>
>>> if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>>>
>>>> + return;
>>>> + }
>>>> + __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);
>>>> @@ -1319,12 +1450,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;
>>>>
>>>> @@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port)
>>>> }
>>>> }
>>>>
>>>> +static void serial8250_em485_handle_start_tx(unsigned long arg)
>>>> +{
>>>> + struct uart_8250_port *p = (struct uart_8250_port *)arg;
>>>> +
>>>> + __start_tx(&p->port);
>>>> +}
>>>> +
>>>> +static void serial8250_start_tx(struct uart_port *port)
>>>> +{
>>>> + struct uart_8250_port *up = up_to_u8250p(port);
>>>> + __u32 delay;
>>>
>>> int delay;
>>>
>>>> +
>>>> + serial8250_rpm_get_tx(up);
>>>> +
>>>> + if (up->em485 && timer_pending(&up->em485->start_tx_timer))
>>>> + return;
>>>> +
>>>> + if (up->em485 && (delay = __start_tx_rs485(up))) {
>>>
>>> No assignment in conditional please.
>>>
>>>> + mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
>>>> + } else {
>>>> + __start_tx(port);
>>>> + }
>>>
>>> Generally, braces aren't used for single statement if..else.
>>> That probably won't apply here after removing the assignment-in-conditional,
>>> but I thought it worth mentioning just so you know.
>>>
>>> Regards,
>>> Peter Hurley
>>>
>>>> +}
>>>> +
>>>> 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..71516ec 100644
>>>> --- a/include/linux/serial_8250.h
>>>> +++ b/include/linux/serial_8250.h
>>>> @@ -76,6 +76,11 @@ 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 */
>>>> +};
>>>> +
>>>> /*
>>>> * This should be used by drivers which want to register
>>>> * their own 8250 ports without registering their own
>>>> @@ -122,6 +127,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)
>>>>
>>>
>>
>>
>>
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
2016-01-15 20:01 ` Matwey V. Kornilov
@ 2016-01-15 21:16 ` Matwey V. Kornilov
2016-01-15 22:17 ` Peter Hurley
0 siblings, 1 reply; 17+ messages in thread
From: Matwey V. Kornilov @ 2016-01-15 21:16 UTC (permalink / raw)
To: Peter Hurley
Cc: Greg KH, Jiri Slaby, Andy Shevchenko, One Thousand Gnomes,
linux-kernel, linux-serial
2016-01-15 23:01 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
> 2016-01-15 22:45 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>> On 01/15/2016 10:42 AM, Matwey V. Kornilov wrote:
>>> 2016-01-15 19:14 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>> On 12/21/2015 10:26 AM, Matwey V. Kornilov 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.
>>>>
>>>> Apologies for the long delay; comments below.
>>>>
>>>>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>>>>> ---
>>>>> drivers/tty/serial/8250/8250.h | 6 ++
>>>>> drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++-
>>>>> include/linux/serial_8250.h | 7 ++
>>>>> 3 files changed, 170 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>>>>> index d54dcd8..0189cb3 100644
>>>>> --- a/drivers/tty/serial/8250/8250.h
>>>>> +++ b/drivers/tty/serial/8250/8250.h
>>>>> @@ -117,6 +117,12 @@ 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);
>>>>> +static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
>>>>> +{
>>>>> + return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
>>>>
>>>> Under what circumstances is p->em485 != NULL but
>>>> (p->port.rs485.flags & SER_RS485_ENABLED) is true?
>>>>
>>>> ISTM, p->em485 is necessary and sufficient to determine if em485 is enabled.
>>>>
>>>> In which case, this function can be eliminated and callers can be reduced to
>>>>
>>>> if (p->em485)
>>>> ....
>>>>
>>>>> +}
>>>>>
>>>>> #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 8ad0b2d..d67a848 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>
>>>>> @@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>>>>> }
>>>>> }
>>>>>
>>>>> +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
>>>>
>>>> Only one call site, so please drop inline.
>>>>
>>>>
>>>>> +{
>>>>> + unsigned char mcr = serial_in(p, UART_MCR);
>>>>> +
>>>>> + if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>>>>> + mcr |= UART_MCR_RTS;
>>>>> + else
>>>>> + mcr &= ~UART_MCR_RTS;
>>>>> + serial_out(p, UART_MCR, mcr);
>>>>> +}
>>>>> +
>>>>> +static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
>>>>
>>>> Doesn't really need to be inline.
>>>>
>>>>
>>>>> +{
>>>>> + 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);
>>>>> @@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>>>>
>>>>> +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;
>>>>> +
>>>>> + init_timer(&p->em485->stop_tx_timer);
>>>>> + p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
>>>>> + p->em485->stop_tx_timer.data = (unsigned long)p;
>>>>> + p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
>>>>
>>>> Not sure this is going to fly; this would be the only user of TIMER_IRQSAFE
>>>> (which was specifically introduced to workaround workqueue issues and not
>>>> meant for general use).
>>>
>>> This is required to call del_timer_sync(&p->em485->start_tx_timer);
>>> from __stop_tx_rs485
>>
>> I know; that doesn't mean it's ok.
>>
>
> What do you suggest? Run __stop_tx as a tasklet? I am not sure whether
> it introduces races or not.
Would it be fine to use workqueues instead of timers? I mean
schedule_delayed_work and cancel_delayed_work_sync.
They use same timers with TIMER_IRQSAFE under the hood.
Or it is better to allocate separate work queue in order to achieve
better latency than shared system wq can provide?
>
>>
>>>>> + init_timer(&p->em485->start_tx_timer);
>>>>> + p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
>>>>> + p->em485->start_tx_timer.data = (unsigned long)p;
>>>>> + p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
>>>>> +
>>>>> + serial8250_em485_rts_after_send(p);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(serial8250_em485_init);
>>>>
>>>> Newline.
>>>>
>>>>> +void serial8250_em485_destroy(struct uart_8250_port *p)
>>>>> +{
>>>>> + if (p->em485 == NULL)
>>>>> + return;
>>>>> +
>>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>>
>>>> What keeps start_tx() from restarting a new timer right here?
>>>
>>> Both start_tx and rs485_config (which calls destroy) are wrapped with
>>> port->lock in serial_core.c
>>
>> Ahh, missed that.
>>
>> Maybe it would be better simply to implement the config_rs485()
>> generically, and just call it from the omap_8250 config_rs485().
>>
>> And put a note about the locking in a function comment header
>>
>> /**
>> * serial8250_config_em485() - rs485 config helper
>> *
>> * ....
>> */
>>
>>
>>
>>>>> + 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
>>>>> @@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
>>>>> serial8250_rpm_put(up);
>>>>> }
>>>>>
>>>>> -static inline void __stop_tx(struct uart_8250_port *p)
>>>>> +static __u32 __start_tx_rs485(struct uart_8250_port *p)
>>>> ^^^^^
>>>> No need to preserve the userspace type here.
>>>>
>>>> The double underline leader in an identifier is typically used to distinguish
>>>> an unlocked version from a locked version. I don't think it's necessary here
>>>> or any of the other newly-introduced functions.
>>>
>>> I use double __ for consistency with __start_tx. Now I have:
>>>
>>> if (up->em485)
>>> __start_tx_rs485(port);
>>> else
>>> __start_tx(port);
>>
>> But __start_tx() is labelled that way to differentiate it from being identified
>> as the start_tx() handler (which is serial8250_start_tx()). IOW, contributors
>> unfamiliar with the 8250 driver itself won't become confused when grepping
>> for start_tx (or at least the idea is to minimize that confusion).
>>
>> start_tx_rs485() doesn't need differentiation, so doesn't require the
>> double __ leader.
>>
>> FWIW, this is consistent and typical elsewhere in the kernel.
>>
>>
>>>>> +{
>>>>> + if (!serial8250_em485_enabled(p))
>>>>> + return 0;
>>>>
>>>> Already checked that em485 was enabled in lone caller.
>>>>
>>>>
>>>>> + if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>>>>> + serial8250_stop_rx(&p->port);
>>>>> +
>>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>>> +
>>>>> + if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
>>>>
>>>> Line too long. And just one negation is sufficient, ie.
>>>>
>>>> if (!(....) !=
>>>> !(....)) {
>>>>
>>>
>>> I would like to keep the double negation, in my opinion it is more
>>> clear to the reader and I believe that the compiler is able to
>>> optimize it.
>>>
>>>>> + serial8250_em485_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)
>>>>
>>>> Does this really need to be inline?
>>>>
>>>
>>> Why not?
>>
>> The expected yardstick for inline is some demonstrable speed improvement;
>> otherwise, size is favored.
>>
>> And __stop_tx() is already inlined in 3 places, which really doesn't
>> need inlining either -- a call/ret is nothing compared to device i/o.
>>
>
> Ok then, probably I am biased with my C++ experience and I am used to
> think that compiler considers `inline` only as a hint.
>
>>
>> Regards,
>> Peter Hurley
>>
>>>>> +{
>>>>> + if (!serial8250_em485_enabled(p))
>>>>> + return;
>>>>> +
>>>>> + serial8250_em485_rts_after_send(p);
>>>>> + /*
>>>>> + * Empty the RX FIFO, we are not interested in anything
>>>>> + * received during the half-duplex transmission.
>>>>> + */
>>>>
>>>> Malformed block comment.
>>>>
>>>> /*
>>>> *
>>>> *
>>>> */
>>>>
>>>>> + 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;
>>>>> +
>>>>> + __do_stop_tx_rs485(p);
>>>>> +}
>>>>> +
>>>>> +static inline void __stop_tx_rs485(struct uart_8250_port *p)
>>>>
>>>> Single caller so drop inline.
>>>>
>>>>> +{
>>>>> + if (!serial8250_em485_enabled(p))
>>>>> + return;
>>>>> +
>>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>>> +
>>>>> + /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
>>>>
>>>> Block comment.
>>>>
>>>>> + if (p->port.rs485.delay_rts_after_send > 0) {
>>>>> + mod_timer(&p->em485->stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
>>>>
>>>> Line too long; please re-format.
>>>> This is one problem with overly long identifiers.
>>>>
>>>>> + } 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;
>>>>> @@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p)
>>>>> }
>>>>> }
>>>>>
>>>>> +static inline void __stop_tx(struct uart_8250_port *p)
>>>>> +{
>>>>> + if (serial8250_em485_enabled(p)) {
>>>>> + 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.
>>>>> + */
>>>>
>>>> Block indent.
>>>>
>>>> This code path should cancel start timer also.
>>>>
>>>>> + if (!((lsr & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
>>>>
>>>> if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>>>>
>>>>> + return;
>>>>> + }
>>>>> + __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);
>>>>> @@ -1319,12 +1450,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;
>>>>>
>>>>> @@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port)
>>>>> }
>>>>> }
>>>>>
>>>>> +static void serial8250_em485_handle_start_tx(unsigned long arg)
>>>>> +{
>>>>> + struct uart_8250_port *p = (struct uart_8250_port *)arg;
>>>>> +
>>>>> + __start_tx(&p->port);
>>>>> +}
>>>>> +
>>>>> +static void serial8250_start_tx(struct uart_port *port)
>>>>> +{
>>>>> + struct uart_8250_port *up = up_to_u8250p(port);
>>>>> + __u32 delay;
>>>>
>>>> int delay;
>>>>
>>>>> +
>>>>> + serial8250_rpm_get_tx(up);
>>>>> +
>>>>> + if (up->em485 && timer_pending(&up->em485->start_tx_timer))
>>>>> + return;
>>>>> +
>>>>> + if (up->em485 && (delay = __start_tx_rs485(up))) {
>>>>
>>>> No assignment in conditional please.
>>>>
>>>>> + mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
>>>>> + } else {
>>>>> + __start_tx(port);
>>>>> + }
>>>>
>>>> Generally, braces aren't used for single statement if..else.
>>>> That probably won't apply here after removing the assignment-in-conditional,
>>>> but I thought it worth mentioning just so you know.
>>>>
>>>> Regards,
>>>> Peter Hurley
>>>>
>>>>> +}
>>>>> +
>>>>> 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..71516ec 100644
>>>>> --- a/include/linux/serial_8250.h
>>>>> +++ b/include/linux/serial_8250.h
>>>>> @@ -76,6 +76,11 @@ 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 */
>>>>> +};
>>>>> +
>>>>> /*
>>>>> * This should be used by drivers which want to register
>>>>> * their own 8250 ports without registering their own
>>>>> @@ -122,6 +127,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)
>>>>>
>>>>
>>>
>>>
>>>
>>
>
>
>
> --
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
2016-01-15 20:20 ` Matwey V. Kornilov
@ 2016-01-15 21:54 ` Peter Hurley
0 siblings, 0 replies; 17+ messages in thread
From: Peter Hurley @ 2016-01-15 21:54 UTC (permalink / raw)
To: Matwey V. Kornilov
Cc: Greg KH, Jiri Slaby, Andy Shevchenko, One Thousand Gnomes,
linux-kernel, linux-serial
On 01/15/2016 12:20 PM, Matwey V. Kornilov wrote:
> 2016-01-15 22:45 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>> On 01/15/2016 10:42 AM, Matwey V. Kornilov wrote:
>>> 2016-01-15 19:14 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>> On 12/21/2015 10:26 AM, Matwey V. Kornilov 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.
>>>>
>>>> Apologies for the long delay; comments below.
>>>>
>>>>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>>>>> ---
>>>>> drivers/tty/serial/8250/8250.h | 6 ++
>>>>> drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++-
>>>>> include/linux/serial_8250.h | 7 ++
>>>>> 3 files changed, 170 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>>>>> index d54dcd8..0189cb3 100644
>>>>> --- a/drivers/tty/serial/8250/8250.h
>>>>> +++ b/drivers/tty/serial/8250/8250.h
>>>>> @@ -117,6 +117,12 @@ 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);
>>>>> +static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
>>>>> +{
>>>>> + return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
>>>>
>>>> Under what circumstances is p->em485 != NULL but
>>>> (p->port.rs485.flags & SER_RS485_ENABLED) is true?
>>>>
>>>> ISTM, p->em485 is necessary and sufficient to determine if em485 is enabled.
>>>>
>>>> In which case, this function can be eliminated and callers can be reduced to
>>>>
>>>> if (p->em485)
>>>> ....
>>>>
>>>>> +}
>>>>>
>>>>> #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 8ad0b2d..d67a848 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>
>>>>> @@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>>>>> }
>>>>> }
>>>>>
>>>>> +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
>>>>
>>>> Only one call site, so please drop inline.
>>>>
>>>>
>>>>> +{
>>>>> + unsigned char mcr = serial_in(p, UART_MCR);
>>>>> +
>>>>> + if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>>>>> + mcr |= UART_MCR_RTS;
>>>>> + else
>>>>> + mcr &= ~UART_MCR_RTS;
>>>>> + serial_out(p, UART_MCR, mcr);
>>>>> +}
>>>>> +
>>>>> +static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
>>>>
>>>> Doesn't really need to be inline.
>>>>
>>>>
>>>>> +{
>>>>> + 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);
>>>>> @@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>>>>
>>>>> +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;
>>>>> +
>>>>> + init_timer(&p->em485->stop_tx_timer);
>>>>> + p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
>>>>> + p->em485->stop_tx_timer.data = (unsigned long)p;
>>>>> + p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
>>>>
>>>> Not sure this is going to fly; this would be the only user of TIMER_IRQSAFE
>>>> (which was specifically introduced to workaround workqueue issues and not
>>>> meant for general use).
>>>
>>> This is required to call del_timer_sync(&p->em485->start_tx_timer);
>>> from __stop_tx_rs485
>>
>> I know; that doesn't mean it's ok.
>>
>>
>>>>> + init_timer(&p->em485->start_tx_timer);
>>>>> + p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
>>>>> + p->em485->start_tx_timer.data = (unsigned long)p;
>>>>> + p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
>>>>> +
>>>>> + serial8250_em485_rts_after_send(p);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(serial8250_em485_init);
>>>>
>>>> Newline.
>>>>
>>>>> +void serial8250_em485_destroy(struct uart_8250_port *p)
>>>>> +{
>>>>> + if (p->em485 == NULL)
>>>>> + return;
>>>>> +
>>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>>
>>>> What keeps start_tx() from restarting a new timer right here?
>>>
>>> Both start_tx and rs485_config (which calls destroy) are wrapped with
>>> port->lock in serial_core.c
>>
>> Ahh, missed that.
>>
>> Maybe it would be better simply to implement the config_rs485()
>> generically, and just call it from the omap_8250 config_rs485().
>>
>> And put a note about the locking in a function comment header
>>
>> /**
>> * serial8250_config_em485() - rs485 config helper
>> *
>> * ....
>> */
>>
>
> I would like to provide init and destroy and left the rest for user.
> The reason is simple.
> When serial8250_em485_init is called a driver has to enable interrupt
> on empty shift register and probably disable it after
> serial8250_em485_destroy.
> And I believe that the driver knows better how to do it
> transactionally and correctly.
Ok.
> The 8250_omap doesn't do that just because it enables the interrupt
> normally on start and always keep it enabled.
Which would be another good note to add to the serial8250_em485_init()
function comment. I know it, and you know it, but someone a while from
now might not. I realize you documented that requirement in __stop_tx()
but someone might not bother to look too closely at the implementation.
Regards,
Peter Hurley
>>>>> + 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
>>>>> @@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
>>>>> serial8250_rpm_put(up);
>>>>> }
>>>>>
>>>>> -static inline void __stop_tx(struct uart_8250_port *p)
>>>>> +static __u32 __start_tx_rs485(struct uart_8250_port *p)
>>>> ^^^^^
>>>> No need to preserve the userspace type here.
>>>>
>>>> The double underline leader in an identifier is typically used to distinguish
>>>> an unlocked version from a locked version. I don't think it's necessary here
>>>> or any of the other newly-introduced functions.
>>>
>>> I use double __ for consistency with __start_tx. Now I have:
>>>
>>> if (up->em485)
>>> __start_tx_rs485(port);
>>> else
>>> __start_tx(port);
>>
>> But __start_tx() is labelled that way to differentiate it from being identified
>> as the start_tx() handler (which is serial8250_start_tx()). IOW, contributors
>> unfamiliar with the 8250 driver itself won't become confused when grepping
>> for start_tx (or at least the idea is to minimize that confusion).
>>
>> start_tx_rs485() doesn't need differentiation, so doesn't require the
>> double __ leader.
>>
>> FWIW, this is consistent and typical elsewhere in the kernel.
>>
>>
>>>>> +{
>>>>> + if (!serial8250_em485_enabled(p))
>>>>> + return 0;
>>>>
>>>> Already checked that em485 was enabled in lone caller.
>>>>
>>>>
>>>>> + if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>>>>> + serial8250_stop_rx(&p->port);
>>>>> +
>>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>>> +
>>>>> + if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
>>>>
>>>> Line too long. And just one negation is sufficient, ie.
>>>>
>>>> if (!(....) !=
>>>> !(....)) {
>>>>
>>>
>>> I would like to keep the double negation, in my opinion it is more
>>> clear to the reader and I believe that the compiler is able to
>>> optimize it.
>>>
>>>>> + serial8250_em485_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)
>>>>
>>>> Does this really need to be inline?
>>>>
>>>
>>> Why not?
>>
>> The expected yardstick for inline is some demonstrable speed improvement;
>> otherwise, size is favored.
>>
>> And __stop_tx() is already inlined in 3 places, which really doesn't
>> need inlining either -- a call/ret is nothing compared to device i/o.
>>
>>
>> Regards,
>> Peter Hurley
>>
>>>>> +{
>>>>> + if (!serial8250_em485_enabled(p))
>>>>> + return;
>>>>> +
>>>>> + serial8250_em485_rts_after_send(p);
>>>>> + /*
>>>>> + * Empty the RX FIFO, we are not interested in anything
>>>>> + * received during the half-duplex transmission.
>>>>> + */
>>>>
>>>> Malformed block comment.
>>>>
>>>> /*
>>>> *
>>>> *
>>>> */
>>>>
>>>>> + 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;
>>>>> +
>>>>> + __do_stop_tx_rs485(p);
>>>>> +}
>>>>> +
>>>>> +static inline void __stop_tx_rs485(struct uart_8250_port *p)
>>>>
>>>> Single caller so drop inline.
>>>>
>>>>> +{
>>>>> + if (!serial8250_em485_enabled(p))
>>>>> + return;
>>>>> +
>>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>>> +
>>>>> + /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
>>>>
>>>> Block comment.
>>>>
>>>>> + if (p->port.rs485.delay_rts_after_send > 0) {
>>>>> + mod_timer(&p->em485->stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
>>>>
>>>> Line too long; please re-format.
>>>> This is one problem with overly long identifiers.
>>>>
>>>>> + } 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;
>>>>> @@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p)
>>>>> }
>>>>> }
>>>>>
>>>>> +static inline void __stop_tx(struct uart_8250_port *p)
>>>>> +{
>>>>> + if (serial8250_em485_enabled(p)) {
>>>>> + 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.
>>>>> + */
>>>>
>>>> Block indent.
>>>>
>>>> This code path should cancel start timer also.
>>>>
>>>>> + if (!((lsr & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
>>>>
>>>> if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>>>>
>>>>> + return;
>>>>> + }
>>>>> + __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);
>>>>> @@ -1319,12 +1450,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;
>>>>>
>>>>> @@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port)
>>>>> }
>>>>> }
>>>>>
>>>>> +static void serial8250_em485_handle_start_tx(unsigned long arg)
>>>>> +{
>>>>> + struct uart_8250_port *p = (struct uart_8250_port *)arg;
>>>>> +
>>>>> + __start_tx(&p->port);
>>>>> +}
>>>>> +
>>>>> +static void serial8250_start_tx(struct uart_port *port)
>>>>> +{
>>>>> + struct uart_8250_port *up = up_to_u8250p(port);
>>>>> + __u32 delay;
>>>>
>>>> int delay;
>>>>
>>>>> +
>>>>> + serial8250_rpm_get_tx(up);
>>>>> +
>>>>> + if (up->em485 && timer_pending(&up->em485->start_tx_timer))
>>>>> + return;
>>>>> +
>>>>> + if (up->em485 && (delay = __start_tx_rs485(up))) {
>>>>
>>>> No assignment in conditional please.
>>>>
>>>>> + mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
>>>>> + } else {
>>>>> + __start_tx(port);
>>>>> + }
>>>>
>>>> Generally, braces aren't used for single statement if..else.
>>>> That probably won't apply here after removing the assignment-in-conditional,
>>>> but I thought it worth mentioning just so you know.
>>>>
>>>> Regards,
>>>> Peter Hurley
>>>>
>>>>> +}
>>>>> +
>>>>> 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..71516ec 100644
>>>>> --- a/include/linux/serial_8250.h
>>>>> +++ b/include/linux/serial_8250.h
>>>>> @@ -76,6 +76,11 @@ 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 */
>>>>> +};
>>>>> +
>>>>> /*
>>>>> * This should be used by drivers which want to register
>>>>> * their own 8250 ports without registering their own
>>>>> @@ -122,6 +127,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)
>>>>>
>>>>
>>>
>>>
>>>
>>
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
2016-01-15 21:16 ` Matwey V. Kornilov
@ 2016-01-15 22:17 ` Peter Hurley
2016-01-16 8:12 ` Matwey V. Kornilov
0 siblings, 1 reply; 17+ messages in thread
From: Peter Hurley @ 2016-01-15 22:17 UTC (permalink / raw)
To: Matwey V. Kornilov
Cc: Greg KH, Jiri Slaby, Andy Shevchenko, One Thousand Gnomes,
linux-kernel, linux-serial
On 01/15/2016 01:16 PM, Matwey V. Kornilov wrote:
> 2016-01-15 23:01 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
>> 2016-01-15 22:45 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>> On 01/15/2016 10:42 AM, Matwey V. Kornilov wrote:
>>>> 2016-01-15 19:14 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>>> On 12/21/2015 10:26 AM, Matwey V. Kornilov 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.
>>>>>
>>>>> Apologies for the long delay; comments below.
>>>>>
>>>>>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>>>>>> ---
>>>>>> drivers/tty/serial/8250/8250.h | 6 ++
>>>>>> drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++-
>>>>>> include/linux/serial_8250.h | 7 ++
>>>>>> 3 files changed, 170 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>>>>>> index d54dcd8..0189cb3 100644
>>>>>> --- a/drivers/tty/serial/8250/8250.h
>>>>>> +++ b/drivers/tty/serial/8250/8250.h
>>>>>> @@ -117,6 +117,12 @@ 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);
>>>>>> +static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
>>>>>> +{
>>>>>> + return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
>>>>>
>>>>> Under what circumstances is p->em485 != NULL but
>>>>> (p->port.rs485.flags & SER_RS485_ENABLED) is true?
>>>>>
>>>>> ISTM, p->em485 is necessary and sufficient to determine if em485 is enabled.
>>>>>
>>>>> In which case, this function can be eliminated and callers can be reduced to
>>>>>
>>>>> if (p->em485)
>>>>> ....
>>>>>
>>>>>> +}
>>>>>>
>>>>>> #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 8ad0b2d..d67a848 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>
>>>>>> @@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
>>>>>
>>>>> Only one call site, so please drop inline.
>>>>>
>>>>>
>>>>>> +{
>>>>>> + unsigned char mcr = serial_in(p, UART_MCR);
>>>>>> +
>>>>>> + if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>>>>>> + mcr |= UART_MCR_RTS;
>>>>>> + else
>>>>>> + mcr &= ~UART_MCR_RTS;
>>>>>> + serial_out(p, UART_MCR, mcr);
>>>>>> +}
>>>>>> +
>>>>>> +static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
>>>>>
>>>>> Doesn't really need to be inline.
>>>>>
>>>>>
>>>>>> +{
>>>>>> + 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);
>>>>>> @@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>>>>>
>>>>>> +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;
>>>>>> +
>>>>>> + init_timer(&p->em485->stop_tx_timer);
>>>>>> + p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
>>>>>> + p->em485->stop_tx_timer.data = (unsigned long)p;
>>>>>> + p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
>>>>>
>>>>> Not sure this is going to fly; this would be the only user of TIMER_IRQSAFE
>>>>> (which was specifically introduced to workaround workqueue issues and not
>>>>> meant for general use).
>>>>
>>>> This is required to call del_timer_sync(&p->em485->start_tx_timer);
>>>> from __stop_tx_rs485
>>>
>>> I know; that doesn't mean it's ok.
>>>
>>
>> What do you suggest? Run __stop_tx as a tasklet? I am not sure whether
>> it introduces races or not.
>
> Would it be fine to use workqueues instead of timers? I mean
> schedule_delayed_work and cancel_delayed_work_sync.
> They use same timers with TIMER_IRQSAFE under the hood.
> Or it is better to allocate separate work queue in order to achieve
> better latency than shared system wq can provide?
I think just del_timer() and locking with the port lock should be
sufficient; timer + irq handler is nothing new.
>>>>>> + init_timer(&p->em485->start_tx_timer);
>>>>>> + p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
>>>>>> + p->em485->start_tx_timer.data = (unsigned long)p;
>>>>>> + p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
>>>>>> +
>>>>>> + serial8250_em485_rts_after_send(p);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(serial8250_em485_init);
>>>>>
>>>>> Newline.
>>>>>
>>>>>> +void serial8250_em485_destroy(struct uart_8250_port *p)
>>>>>> +{
>>>>>> + if (p->em485 == NULL)
>>>>>> + return;
>>>>>> +
>>>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>>>
>>>>> What keeps start_tx() from restarting a new timer right here?
>>>>
>>>> Both start_tx and rs485_config (which calls destroy) are wrapped with
>>>> port->lock in serial_core.c
>>>
>>> Ahh, missed that.
>>>
>>> Maybe it would be better simply to implement the config_rs485()
>>> generically, and just call it from the omap_8250 config_rs485().
>>>
>>> And put a note about the locking in a function comment header
>>>
>>> /**
>>> * serial8250_config_em485() - rs485 config helper
>>> *
>>> * ....
>>> */
>>>
>>>
>>>
>>>>>> + 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
>>>>>> @@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
>>>>>> serial8250_rpm_put(up);
>>>>>> }
>>>>>>
>>>>>> -static inline void __stop_tx(struct uart_8250_port *p)
>>>>>> +static __u32 __start_tx_rs485(struct uart_8250_port *p)
>>>>> ^^^^^
>>>>> No need to preserve the userspace type here.
>>>>>
>>>>> The double underline leader in an identifier is typically used to distinguish
>>>>> an unlocked version from a locked version. I don't think it's necessary here
>>>>> or any of the other newly-introduced functions.
>>>>
>>>> I use double __ for consistency with __start_tx. Now I have:
>>>>
>>>> if (up->em485)
>>>> __start_tx_rs485(port);
>>>> else
>>>> __start_tx(port);
>>>
>>> But __start_tx() is labelled that way to differentiate it from being identified
>>> as the start_tx() handler (which is serial8250_start_tx()). IOW, contributors
>>> unfamiliar with the 8250 driver itself won't become confused when grepping
>>> for start_tx (or at least the idea is to minimize that confusion).
>>>
>>> start_tx_rs485() doesn't need differentiation, so doesn't require the
>>> double __ leader.
>>>
>>> FWIW, this is consistent and typical elsewhere in the kernel.
>>>
>>>
>>>>>> +{
>>>>>> + if (!serial8250_em485_enabled(p))
>>>>>> + return 0;
>>>>>
>>>>> Already checked that em485 was enabled in lone caller.
>>>>>
>>>>>
>>>>>> + if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>>>>>> + serial8250_stop_rx(&p->port);
>>>>>> +
>>>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>>>> +
>>>>>> + if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
>>>>>
>>>>> Line too long. And just one negation is sufficient, ie.
>>>>>
>>>>> if (!(....) !=
>>>>> !(....)) {
>>>>>
>>>>
>>>> I would like to keep the double negation, in my opinion it is more
>>>> clear to the reader and I believe that the compiler is able to
>>>> optimize it.
>>>>
>>>>>> + serial8250_em485_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)
>>>>>
>>>>> Does this really need to be inline?
>>>>>
>>>>
>>>> Why not?
>>>
>>> The expected yardstick for inline is some demonstrable speed improvement;
>>> otherwise, size is favored.
>>>
>>> And __stop_tx() is already inlined in 3 places, which really doesn't
>>> need inlining either -- a call/ret is nothing compared to device i/o.
>>>
>>
>> Ok then, probably I am biased with my C++ experience and I am used to
>> think that compiler considers `inline` only as a hint.
>>
>>>
>>> Regards,
>>> Peter Hurley
>>>
>>>>>> +{
>>>>>> + if (!serial8250_em485_enabled(p))
>>>>>> + return;
>>>>>> +
>>>>>> + serial8250_em485_rts_after_send(p);
>>>>>> + /*
>>>>>> + * Empty the RX FIFO, we are not interested in anything
>>>>>> + * received during the half-duplex transmission.
>>>>>> + */
>>>>>
>>>>> Malformed block comment.
>>>>>
>>>>> /*
>>>>> *
>>>>> *
>>>>> */
>>>>>
>>>>>> + 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;
>>>>>> +
>>>>>> + __do_stop_tx_rs485(p);
>>>>>> +}
>>>>>> +
>>>>>> +static inline void __stop_tx_rs485(struct uart_8250_port *p)
>>>>>
>>>>> Single caller so drop inline.
>>>>>
>>>>>> +{
>>>>>> + if (!serial8250_em485_enabled(p))
>>>>>> + return;
>>>>>> +
>>>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>>>> +
>>>>>> + /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
>>>>>
>>>>> Block comment.
>>>>>
>>>>>> + if (p->port.rs485.delay_rts_after_send > 0) {
>>>>>> + mod_timer(&p->em485->stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
>>>>>
>>>>> Line too long; please re-format.
>>>>> This is one problem with overly long identifiers.
>>>>>
>>>>>> + } 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;
>>>>>> @@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p)
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> +static inline void __stop_tx(struct uart_8250_port *p)
>>>>>> +{
>>>>>> + if (serial8250_em485_enabled(p)) {
>>>>>> + 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.
>>>>>> + */
>>>>>
>>>>> Block indent.
>>>>>
>>>>> This code path should cancel start timer also.
>>>>>
>>>>>> + if (!((lsr & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
>>>>>
>>>>> if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>>>>>
>>>>>> + return;
>>>>>> + }
>>>>>> + __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);
>>>>>> @@ -1319,12 +1450,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;
>>>>>>
>>>>>> @@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port)
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> +static void serial8250_em485_handle_start_tx(unsigned long arg)
>>>>>> +{
>>>>>> + struct uart_8250_port *p = (struct uart_8250_port *)arg;
>>>>>> +
>>>>>> + __start_tx(&p->port);
>>>>>> +}
>>>>>> +
>>>>>> +static void serial8250_start_tx(struct uart_port *port)
>>>>>> +{
>>>>>> + struct uart_8250_port *up = up_to_u8250p(port);
>>>>>> + __u32 delay;
>>>>>
>>>>> int delay;
>>>>>
>>>>>> +
>>>>>> + serial8250_rpm_get_tx(up);
>>>>>> +
>>>>>> + if (up->em485 && timer_pending(&up->em485->start_tx_timer))
>>>>>> + return;
>>>>>> +
>>>>>> + if (up->em485 && (delay = __start_tx_rs485(up))) {
>>>>>
>>>>> No assignment in conditional please.
>>>>>
>>>>>> + mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
>>>>>> + } else {
>>>>>> + __start_tx(port);
>>>>>> + }
>>>>>
>>>>> Generally, braces aren't used for single statement if..else.
>>>>> That probably won't apply here after removing the assignment-in-conditional,
>>>>> but I thought it worth mentioning just so you know.
>>>>>
>>>>> Regards,
>>>>> Peter Hurley
>>>>>
>>>>>> +}
>>>>>> +
>>>>>> 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..71516ec 100644
>>>>>> --- a/include/linux/serial_8250.h
>>>>>> +++ b/include/linux/serial_8250.h
>>>>>> @@ -76,6 +76,11 @@ 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 */
>>>>>> +};
>>>>>> +
>>>>>> /*
>>>>>> * This should be used by drivers which want to register
>>>>>> * their own 8250 ports without registering their own
>>>>>> @@ -122,6 +127,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)
>>>>>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
2016-01-15 22:17 ` Peter Hurley
@ 2016-01-16 8:12 ` Matwey V. Kornilov
2016-01-16 18:56 ` Peter Hurley
0 siblings, 1 reply; 17+ messages in thread
From: Matwey V. Kornilov @ 2016-01-16 8:12 UTC (permalink / raw)
To: Peter Hurley
Cc: Greg KH, Jiri Slaby, Andy Shevchenko, One Thousand Gnomes,
linux-kernel, linux-serial
2016-01-16 1:17 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> On 01/15/2016 01:16 PM, Matwey V. Kornilov wrote:
>> 2016-01-15 23:01 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
>>> 2016-01-15 22:45 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>> On 01/15/2016 10:42 AM, Matwey V. Kornilov wrote:
>>>>> 2016-01-15 19:14 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>>>> On 12/21/2015 10:26 AM, Matwey V. Kornilov 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.
>>>>>>
>>>>>> Apologies for the long delay; comments below.
>>>>>>
>>>>>>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>>>>>>> ---
>>>>>>> drivers/tty/serial/8250/8250.h | 6 ++
>>>>>>> drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++-
>>>>>>> include/linux/serial_8250.h | 7 ++
>>>>>>> 3 files changed, 170 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>>>>>>> index d54dcd8..0189cb3 100644
>>>>>>> --- a/drivers/tty/serial/8250/8250.h
>>>>>>> +++ b/drivers/tty/serial/8250/8250.h
>>>>>>> @@ -117,6 +117,12 @@ 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);
>>>>>>> +static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
>>>>>>> +{
>>>>>>> + return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
>>>>>>
>>>>>> Under what circumstances is p->em485 != NULL but
>>>>>> (p->port.rs485.flags & SER_RS485_ENABLED) is true?
>>>>>>
>>>>>> ISTM, p->em485 is necessary and sufficient to determine if em485 is enabled.
>>>>>>
>>>>>> In which case, this function can be eliminated and callers can be reduced to
>>>>>>
>>>>>> if (p->em485)
>>>>>> ....
>>>>>>
>>>>>>> +}
>>>>>>>
>>>>>>> #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 8ad0b2d..d67a848 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>
>>>>>>> @@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
>>>>>>
>>>>>> Only one call site, so please drop inline.
>>>>>>
>>>>>>
>>>>>>> +{
>>>>>>> + unsigned char mcr = serial_in(p, UART_MCR);
>>>>>>> +
>>>>>>> + if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>>>>>>> + mcr |= UART_MCR_RTS;
>>>>>>> + else
>>>>>>> + mcr &= ~UART_MCR_RTS;
>>>>>>> + serial_out(p, UART_MCR, mcr);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
>>>>>>
>>>>>> Doesn't really need to be inline.
>>>>>>
>>>>>>
>>>>>>> +{
>>>>>>> + 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);
>>>>>>> @@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
>>>>>>> }
>>>>>>> EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>>>>>>
>>>>>>> +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;
>>>>>>> +
>>>>>>> + init_timer(&p->em485->stop_tx_timer);
>>>>>>> + p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
>>>>>>> + p->em485->stop_tx_timer.data = (unsigned long)p;
>>>>>>> + p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
>>>>>>
>>>>>> Not sure this is going to fly; this would be the only user of TIMER_IRQSAFE
>>>>>> (which was specifically introduced to workaround workqueue issues and not
>>>>>> meant for general use).
>>>>>
>>>>> This is required to call del_timer_sync(&p->em485->start_tx_timer);
>>>>> from __stop_tx_rs485
>>>>
>>>> I know; that doesn't mean it's ok.
>>>>
>>>
>>> What do you suggest? Run __stop_tx as a tasklet? I am not sure whether
>>> it introduces races or not.
>>
>> Would it be fine to use workqueues instead of timers? I mean
>> schedule_delayed_work and cancel_delayed_work_sync.
>> They use same timers with TIMER_IRQSAFE under the hood.
>> Or it is better to allocate separate work queue in order to achieve
>> better latency than shared system wq can provide?
>
> I think just del_timer() and locking with the port lock should be
> sufficient; timer + irq handler is nothing new.
>
Do I understand correctly, that internals of
serial8250_em485_handle_stop_tx and serial8250_em485_handle_start_tx
should be wrapped with port->lock in order to ensure that they are not
running during the call going to run del_timer?
>
>>>>>>> + init_timer(&p->em485->start_tx_timer);
>>>>>>> + p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
>>>>>>> + p->em485->start_tx_timer.data = (unsigned long)p;
>>>>>>> + p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
>>>>>>> +
>>>>>>> + serial8250_em485_rts_after_send(p);
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL_GPL(serial8250_em485_init);
>>>>>>
>>>>>> Newline.
>>>>>>
>>>>>>> +void serial8250_em485_destroy(struct uart_8250_port *p)
>>>>>>> +{
>>>>>>> + if (p->em485 == NULL)
>>>>>>> + return;
>>>>>>> +
>>>>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>>>>
>>>>>> What keeps start_tx() from restarting a new timer right here?
>>>>>
>>>>> Both start_tx and rs485_config (which calls destroy) are wrapped with
>>>>> port->lock in serial_core.c
>>>>
>>>> Ahh, missed that.
>>>>
>>>> Maybe it would be better simply to implement the config_rs485()
>>>> generically, and just call it from the omap_8250 config_rs485().
>>>>
>>>> And put a note about the locking in a function comment header
>>>>
>>>> /**
>>>> * serial8250_config_em485() - rs485 config helper
>>>> *
>>>> * ....
>>>> */
>>>>
>>>>
>>>>
>>>>>>> + 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
>>>>>>> @@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
>>>>>>> serial8250_rpm_put(up);
>>>>>>> }
>>>>>>>
>>>>>>> -static inline void __stop_tx(struct uart_8250_port *p)
>>>>>>> +static __u32 __start_tx_rs485(struct uart_8250_port *p)
>>>>>> ^^^^^
>>>>>> No need to preserve the userspace type here.
>>>>>>
>>>>>> The double underline leader in an identifier is typically used to distinguish
>>>>>> an unlocked version from a locked version. I don't think it's necessary here
>>>>>> or any of the other newly-introduced functions.
>>>>>
>>>>> I use double __ for consistency with __start_tx. Now I have:
>>>>>
>>>>> if (up->em485)
>>>>> __start_tx_rs485(port);
>>>>> else
>>>>> __start_tx(port);
>>>>
>>>> But __start_tx() is labelled that way to differentiate it from being identified
>>>> as the start_tx() handler (which is serial8250_start_tx()). IOW, contributors
>>>> unfamiliar with the 8250 driver itself won't become confused when grepping
>>>> for start_tx (or at least the idea is to minimize that confusion).
>>>>
>>>> start_tx_rs485() doesn't need differentiation, so doesn't require the
>>>> double __ leader.
>>>>
>>>> FWIW, this is consistent and typical elsewhere in the kernel.
>>>>
>>>>
>>>>>>> +{
>>>>>>> + if (!serial8250_em485_enabled(p))
>>>>>>> + return 0;
>>>>>>
>>>>>> Already checked that em485 was enabled in lone caller.
>>>>>>
>>>>>>
>>>>>>> + if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>>>>>>> + serial8250_stop_rx(&p->port);
>>>>>>> +
>>>>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>>>>> +
>>>>>>> + if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
>>>>>>
>>>>>> Line too long. And just one negation is sufficient, ie.
>>>>>>
>>>>>> if (!(....) !=
>>>>>> !(....)) {
>>>>>>
>>>>>
>>>>> I would like to keep the double negation, in my opinion it is more
>>>>> clear to the reader and I believe that the compiler is able to
>>>>> optimize it.
>>>>>
>>>>>>> + serial8250_em485_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)
>>>>>>
>>>>>> Does this really need to be inline?
>>>>>>
>>>>>
>>>>> Why not?
>>>>
>>>> The expected yardstick for inline is some demonstrable speed improvement;
>>>> otherwise, size is favored.
>>>>
>>>> And __stop_tx() is already inlined in 3 places, which really doesn't
>>>> need inlining either -- a call/ret is nothing compared to device i/o.
>>>>
>>>
>>> Ok then, probably I am biased with my C++ experience and I am used to
>>> think that compiler considers `inline` only as a hint.
>>>
>>>>
>>>> Regards,
>>>> Peter Hurley
>>>>
>>>>>>> +{
>>>>>>> + if (!serial8250_em485_enabled(p))
>>>>>>> + return;
>>>>>>> +
>>>>>>> + serial8250_em485_rts_after_send(p);
>>>>>>> + /*
>>>>>>> + * Empty the RX FIFO, we are not interested in anything
>>>>>>> + * received during the half-duplex transmission.
>>>>>>> + */
>>>>>>
>>>>>> Malformed block comment.
>>>>>>
>>>>>> /*
>>>>>> *
>>>>>> *
>>>>>> */
>>>>>>
>>>>>>> + 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;
>>>>>>> +
>>>>>>> + __do_stop_tx_rs485(p);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static inline void __stop_tx_rs485(struct uart_8250_port *p)
>>>>>>
>>>>>> Single caller so drop inline.
>>>>>>
>>>>>>> +{
>>>>>>> + if (!serial8250_em485_enabled(p))
>>>>>>> + return;
>>>>>>> +
>>>>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>>>>> +
>>>>>>> + /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
>>>>>>
>>>>>> Block comment.
>>>>>>
>>>>>>> + if (p->port.rs485.delay_rts_after_send > 0) {
>>>>>>> + mod_timer(&p->em485->stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
>>>>>>
>>>>>> Line too long; please re-format.
>>>>>> This is one problem with overly long identifiers.
>>>>>>
>>>>>>> + } 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;
>>>>>>> @@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p)
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> +static inline void __stop_tx(struct uart_8250_port *p)
>>>>>>> +{
>>>>>>> + if (serial8250_em485_enabled(p)) {
>>>>>>> + 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.
>>>>>>> + */
>>>>>>
>>>>>> Block indent.
>>>>>>
>>>>>> This code path should cancel start timer also.
>>>>>>
>>>>>>> + if (!((lsr & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
>>>>>>
>>>>>> if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>>>>>>
>>>>>>> + return;
>>>>>>> + }
>>>>>>> + __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);
>>>>>>> @@ -1319,12 +1450,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;
>>>>>>>
>>>>>>> @@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port)
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> +static void serial8250_em485_handle_start_tx(unsigned long arg)
>>>>>>> +{
>>>>>>> + struct uart_8250_port *p = (struct uart_8250_port *)arg;
>>>>>>> +
>>>>>>> + __start_tx(&p->port);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void serial8250_start_tx(struct uart_port *port)
>>>>>>> +{
>>>>>>> + struct uart_8250_port *up = up_to_u8250p(port);
>>>>>>> + __u32 delay;
>>>>>>
>>>>>> int delay;
>>>>>>
>>>>>>> +
>>>>>>> + serial8250_rpm_get_tx(up);
>>>>>>> +
>>>>>>> + if (up->em485 && timer_pending(&up->em485->start_tx_timer))
>>>>>>> + return;
>>>>>>> +
>>>>>>> + if (up->em485 && (delay = __start_tx_rs485(up))) {
>>>>>>
>>>>>> No assignment in conditional please.
>>>>>>
>>>>>>> + mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
>>>>>>> + } else {
>>>>>>> + __start_tx(port);
>>>>>>> + }
>>>>>>
>>>>>> Generally, braces aren't used for single statement if..else.
>>>>>> That probably won't apply here after removing the assignment-in-conditional,
>>>>>> but I thought it worth mentioning just so you know.
>>>>>>
>>>>>> Regards,
>>>>>> Peter Hurley
>>>>>>
>>>>>>> +}
>>>>>>> +
>>>>>>> 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..71516ec 100644
>>>>>>> --- a/include/linux/serial_8250.h
>>>>>>> +++ b/include/linux/serial_8250.h
>>>>>>> @@ -76,6 +76,11 @@ 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 */
>>>>>>> +};
>>>>>>> +
>>>>>>> /*
>>>>>>> * This should be used by drivers which want to register
>>>>>>> * their own 8250 ports without registering their own
>>>>>>> @@ -122,6 +127,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)
>>>>>>>
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
2016-01-16 8:12 ` Matwey V. Kornilov
@ 2016-01-16 18:56 ` Peter Hurley
2016-01-16 20:28 ` Matwey V. Kornilov
0 siblings, 1 reply; 17+ messages in thread
From: Peter Hurley @ 2016-01-16 18:56 UTC (permalink / raw)
To: Matwey V. Kornilov
Cc: Greg KH, Jiri Slaby, Andy Shevchenko, One Thousand Gnomes,
linux-kernel, linux-serial
On 01/16/2016 12:12 AM, Matwey V. Kornilov wrote:
> 2016-01-16 1:17 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>> On 01/15/2016 01:16 PM, Matwey V. Kornilov wrote:
>>> 2016-01-15 23:01 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
>>>> 2016-01-15 22:45 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>>> On 01/15/2016 10:42 AM, Matwey V. Kornilov wrote:
>>>>>> 2016-01-15 19:14 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>>>>> On 12/21/2015 10:26 AM, Matwey V. Kornilov 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.
>>>>>>>
>>>>>>> Apologies for the long delay; comments below.
>>>>>>>
>>>>>>>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>>>>>>>> ---
>>>>>>>> drivers/tty/serial/8250/8250.h | 6 ++
>>>>>>>> drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++-
>>>>>>>> include/linux/serial_8250.h | 7 ++
>>>>>>>> 3 files changed, 170 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>>>>>>>> index d54dcd8..0189cb3 100644
>>>>>>>> --- a/drivers/tty/serial/8250/8250.h
>>>>>>>> +++ b/drivers/tty/serial/8250/8250.h
>>>>>>>> @@ -117,6 +117,12 @@ 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);
>>>>>>>> +static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
>>>>>>>> +{
>>>>>>>> + return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
>>>>>>>
>>>>>>> Under what circumstances is p->em485 != NULL but
>>>>>>> (p->port.rs485.flags & SER_RS485_ENABLED) is true?
>>>>>>>
>>>>>>> ISTM, p->em485 is necessary and sufficient to determine if em485 is enabled.
>>>>>>>
>>>>>>> In which case, this function can be eliminated and callers can be reduced to
>>>>>>>
>>>>>>> if (p->em485)
>>>>>>> ....
>>>>>>>
>>>>>>>> +}
>>>>>>>>
>>>>>>>> #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 8ad0b2d..d67a848 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>
>>>>>>>> @@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>> +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
>>>>>>>
>>>>>>> Only one call site, so please drop inline.
>>>>>>>
>>>>>>>
>>>>>>>> +{
>>>>>>>> + unsigned char mcr = serial_in(p, UART_MCR);
>>>>>>>> +
>>>>>>>> + if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>>>>>>>> + mcr |= UART_MCR_RTS;
>>>>>>>> + else
>>>>>>>> + mcr &= ~UART_MCR_RTS;
>>>>>>>> + serial_out(p, UART_MCR, mcr);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
>>>>>>>
>>>>>>> Doesn't really need to be inline.
>>>>>>>
>>>>>>>
>>>>>>>> +{
>>>>>>>> + 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);
>>>>>>>> @@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
>>>>>>>> }
>>>>>>>> EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>>>>>>>
>>>>>>>> +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;
>>>>>>>> +
>>>>>>>> + init_timer(&p->em485->stop_tx_timer);
>>>>>>>> + p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
>>>>>>>> + p->em485->stop_tx_timer.data = (unsigned long)p;
>>>>>>>> + p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
>>>>>>>
>>>>>>> Not sure this is going to fly; this would be the only user of TIMER_IRQSAFE
>>>>>>> (which was specifically introduced to workaround workqueue issues and not
>>>>>>> meant for general use).
>>>>>>
>>>>>> This is required to call del_timer_sync(&p->em485->start_tx_timer);
>>>>>> from __stop_tx_rs485
>>>>>
>>>>> I know; that doesn't mean it's ok.
>>>>>
>>>>
>>>> What do you suggest? Run __stop_tx as a tasklet? I am not sure whether
>>>> it introduces races or not.
>>>
>>> Would it be fine to use workqueues instead of timers? I mean
>>> schedule_delayed_work and cancel_delayed_work_sync.
>>> They use same timers with TIMER_IRQSAFE under the hood.
>>> Or it is better to allocate separate work queue in order to achieve
>>> better latency than shared system wq can provide?
>>
>> I think just del_timer() and locking with the port lock should be
>> sufficient; timer + irq handler is nothing new.
>>
>
> Do I understand correctly, that internals of
> serial8250_em485_handle_stop_tx and serial8250_em485_handle_start_tx
> should be wrapped with port->lock in order to ensure that they are not
> running during the call going to run del_timer?
Yes.
Of course, you'll need some state mechanism to know in the timer function
that the timer was cancelled. For example, in this situation
CPU 0 CPU 1
start_tx_rs485() [timer fires]
del_timer(stop_tx_timer)
handle_stop_tx()
spin_lock_irqsave(port lock)
*waits*
rts_on_send()
mod_timer(start_tx_timer)
*claims port lock*
* obviously would be bad if *
* do_stop_tx_rs485() ran now *
>>>>>>>> + init_timer(&p->em485->start_tx_timer);
>>>>>>>> + p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
>>>>>>>> + p->em485->start_tx_timer.data = (unsigned long)p;
>>>>>>>> + p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
>>>>>>>> +
>>>>>>>> + serial8250_em485_rts_after_send(p);
>>>>>>>> +
>>>>>>>> + return 0;
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL_GPL(serial8250_em485_init);
>>>>>>>
>>>>>>> Newline.
>>>>>>>
>>>>>>>> +void serial8250_em485_destroy(struct uart_8250_port *p)
>>>>>>>> +{
>>>>>>>> + if (p->em485 == NULL)
>>>>>>>> + return;
>>>>>>>> +
>>>>>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>>>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>>>>>
>>>>>>> What keeps start_tx() from restarting a new timer right here?
>>>>>>
>>>>>> Both start_tx and rs485_config (which calls destroy) are wrapped with
>>>>>> port->lock in serial_core.c
>>>>>
>>>>> Ahh, missed that.
>>>>>
>>>>> Maybe it would be better simply to implement the config_rs485()
>>>>> generically, and just call it from the omap_8250 config_rs485().
>>>>>
>>>>> And put a note about the locking in a function comment header
>>>>>
>>>>> /**
>>>>> * serial8250_config_em485() - rs485 config helper
>>>>> *
>>>>> * ....
>>>>> */
>>>>>
>>>>>
>>>>>
>>>>>>>> + 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
>>>>>>>> @@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
>>>>>>>> serial8250_rpm_put(up);
>>>>>>>> }
>>>>>>>>
>>>>>>>> -static inline void __stop_tx(struct uart_8250_port *p)
>>>>>>>> +static __u32 __start_tx_rs485(struct uart_8250_port *p)
>>>>>>> ^^^^^
>>>>>>> No need to preserve the userspace type here.
>>>>>>>
>>>>>>> The double underline leader in an identifier is typically used to distinguish
>>>>>>> an unlocked version from a locked version. I don't think it's necessary here
>>>>>>> or any of the other newly-introduced functions.
>>>>>>
>>>>>> I use double __ for consistency with __start_tx. Now I have:
>>>>>>
>>>>>> if (up->em485)
>>>>>> __start_tx_rs485(port);
>>>>>> else
>>>>>> __start_tx(port);
>>>>>
>>>>> But __start_tx() is labelled that way to differentiate it from being identified
>>>>> as the start_tx() handler (which is serial8250_start_tx()). IOW, contributors
>>>>> unfamiliar with the 8250 driver itself won't become confused when grepping
>>>>> for start_tx (or at least the idea is to minimize that confusion).
>>>>>
>>>>> start_tx_rs485() doesn't need differentiation, so doesn't require the
>>>>> double __ leader.
>>>>>
>>>>> FWIW, this is consistent and typical elsewhere in the kernel.
>>>>>
>>>>>
>>>>>>>> +{
>>>>>>>> + if (!serial8250_em485_enabled(p))
>>>>>>>> + return 0;
>>>>>>>
>>>>>>> Already checked that em485 was enabled in lone caller.
>>>>>>>
>>>>>>>
>>>>>>>> + if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>>>>>>>> + serial8250_stop_rx(&p->port);
>>>>>>>> +
>>>>>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>>>>>> +
>>>>>>>> + if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
>>>>>>>
>>>>>>> Line too long. And just one negation is sufficient, ie.
>>>>>>>
>>>>>>> if (!(....) !=
>>>>>>> !(....)) {
>>>>>>>
>>>>>>
>>>>>> I would like to keep the double negation, in my opinion it is more
>>>>>> clear to the reader and I believe that the compiler is able to
>>>>>> optimize it.
>>>>>>
>>>>>>>> + serial8250_em485_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)
>>>>>>>
>>>>>>> Does this really need to be inline?
>>>>>>>
>>>>>>
>>>>>> Why not?
>>>>>
>>>>> The expected yardstick for inline is some demonstrable speed improvement;
>>>>> otherwise, size is favored.
>>>>>
>>>>> And __stop_tx() is already inlined in 3 places, which really doesn't
>>>>> need inlining either -- a call/ret is nothing compared to device i/o.
>>>>>
>>>>
>>>> Ok then, probably I am biased with my C++ experience and I am used to
>>>> think that compiler considers `inline` only as a hint.
>>>>
>>>>>
>>>>> Regards,
>>>>> Peter Hurley
>>>>>
>>>>>>>> +{
>>>>>>>> + if (!serial8250_em485_enabled(p))
>>>>>>>> + return;
>>>>>>>> +
>>>>>>>> + serial8250_em485_rts_after_send(p);
>>>>>>>> + /*
>>>>>>>> + * Empty the RX FIFO, we are not interested in anything
>>>>>>>> + * received during the half-duplex transmission.
>>>>>>>> + */
>>>>>>>
>>>>>>> Malformed block comment.
>>>>>>>
>>>>>>> /*
>>>>>>> *
>>>>>>> *
>>>>>>> */
>>>>>>>
>>>>>>>> + 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;
>>>>>>>> +
>>>>>>>> + __do_stop_tx_rs485(p);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static inline void __stop_tx_rs485(struct uart_8250_port *p)
>>>>>>>
>>>>>>> Single caller so drop inline.
>>>>>>>
>>>>>>>> +{
>>>>>>>> + if (!serial8250_em485_enabled(p))
>>>>>>>> + return;
>>>>>>>> +
>>>>>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>>>>>> +
>>>>>>>> + /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
>>>>>>>
>>>>>>> Block comment.
>>>>>>>
>>>>>>>> + if (p->port.rs485.delay_rts_after_send > 0) {
>>>>>>>> + mod_timer(&p->em485->stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
>>>>>>>
>>>>>>> Line too long; please re-format.
>>>>>>> This is one problem with overly long identifiers.
>>>>>>>
>>>>>>>> + } 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;
>>>>>>>> @@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p)
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>> +static inline void __stop_tx(struct uart_8250_port *p)
>>>>>>>> +{
>>>>>>>> + if (serial8250_em485_enabled(p)) {
>>>>>>>> + 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.
>>>>>>>> + */
>>>>>>>
>>>>>>> Block indent.
>>>>>>>
>>>>>>> This code path should cancel start timer also.
>>>>>>>
>>>>>>>> + if (!((lsr & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
>>>>>>>
>>>>>>> if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>>>>>>>
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> + __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);
>>>>>>>> @@ -1319,12 +1450,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;
>>>>>>>>
>>>>>>>> @@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port)
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>> +static void serial8250_em485_handle_start_tx(unsigned long arg)
>>>>>>>> +{
>>>>>>>> + struct uart_8250_port *p = (struct uart_8250_port *)arg;
>>>>>>>> +
>>>>>>>> + __start_tx(&p->port);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void serial8250_start_tx(struct uart_port *port)
>>>>>>>> +{
>>>>>>>> + struct uart_8250_port *up = up_to_u8250p(port);
>>>>>>>> + __u32 delay;
>>>>>>>
>>>>>>> int delay;
>>>>>>>
>>>>>>>> +
>>>>>>>> + serial8250_rpm_get_tx(up);
>>>>>>>> +
>>>>>>>> + if (up->em485 && timer_pending(&up->em485->start_tx_timer))
>>>>>>>> + return;
>>>>>>>> +
>>>>>>>> + if (up->em485 && (delay = __start_tx_rs485(up))) {
>>>>>>>
>>>>>>> No assignment in conditional please.
>>>>>>>
>>>>>>>> + mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
>>>>>>>> + } else {
>>>>>>>> + __start_tx(port);
>>>>>>>> + }
>>>>>>>
>>>>>>> Generally, braces aren't used for single statement if..else.
>>>>>>> That probably won't apply here after removing the assignment-in-conditional,
>>>>>>> but I thought it worth mentioning just so you know.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Peter Hurley
>>>>>>>
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> 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..71516ec 100644
>>>>>>>> --- a/include/linux/serial_8250.h
>>>>>>>> +++ b/include/linux/serial_8250.h
>>>>>>>> @@ -76,6 +76,11 @@ 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 */
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> /*
>>>>>>>> * This should be used by drivers which want to register
>>>>>>>> * their own 8250 ports without registering their own
>>>>>>>> @@ -122,6 +127,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)
>>>>>>>>
>>
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
2016-01-16 18:56 ` Peter Hurley
@ 2016-01-16 20:28 ` Matwey V. Kornilov
0 siblings, 0 replies; 17+ messages in thread
From: Matwey V. Kornilov @ 2016-01-16 20:28 UTC (permalink / raw)
To: Peter Hurley
Cc: Greg KH, Jiri Slaby, Andy Shevchenko, One Thousand Gnomes,
linux-kernel, linux-serial
2016-01-16 21:56 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> On 01/16/2016 12:12 AM, Matwey V. Kornilov wrote:
>> 2016-01-16 1:17 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>> On 01/15/2016 01:16 PM, Matwey V. Kornilov wrote:
>>>> 2016-01-15 23:01 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
>>>>> 2016-01-15 22:45 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>>>> On 01/15/2016 10:42 AM, Matwey V. Kornilov wrote:
>>>>>>> 2016-01-15 19:14 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>>>>>> On 12/21/2015 10:26 AM, Matwey V. Kornilov 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.
>>>>>>>>
>>>>>>>> Apologies for the long delay; comments below.
>>>>>>>>
>>>>>>>>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>>>>>>>>> ---
>>>>>>>>> drivers/tty/serial/8250/8250.h | 6 ++
>>>>>>>>> drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++-
>>>>>>>>> include/linux/serial_8250.h | 7 ++
>>>>>>>>> 3 files changed, 170 insertions(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>>>>>>>>> index d54dcd8..0189cb3 100644
>>>>>>>>> --- a/drivers/tty/serial/8250/8250.h
>>>>>>>>> +++ b/drivers/tty/serial/8250/8250.h
>>>>>>>>> @@ -117,6 +117,12 @@ 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);
>>>>>>>>> +static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
>>>>>>>>> +{
>>>>>>>>> + return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
>>>>>>>>
>>>>>>>> Under what circumstances is p->em485 != NULL but
>>>>>>>> (p->port.rs485.flags & SER_RS485_ENABLED) is true?
>>>>>>>>
>>>>>>>> ISTM, p->em485 is necessary and sufficient to determine if em485 is enabled.
>>>>>>>>
>>>>>>>> In which case, this function can be eliminated and callers can be reduced to
>>>>>>>>
>>>>>>>> if (p->em485)
>>>>>>>> ....
>>>>>>>>
>>>>>>>>> +}
>>>>>>>>>
>>>>>>>>> #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 8ad0b2d..d67a848 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>
>>>>>>>>> @@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
>>>>>>>>
>>>>>>>> Only one call site, so please drop inline.
>>>>>>>>
>>>>>>>>
>>>>>>>>> +{
>>>>>>>>> + unsigned char mcr = serial_in(p, UART_MCR);
>>>>>>>>> +
>>>>>>>>> + if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>>>>>>>>> + mcr |= UART_MCR_RTS;
>>>>>>>>> + else
>>>>>>>>> + mcr &= ~UART_MCR_RTS;
>>>>>>>>> + serial_out(p, UART_MCR, mcr);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
>>>>>>>>
>>>>>>>> Doesn't really need to be inline.
>>>>>>>>
>>>>>>>>
>>>>>>>>> +{
>>>>>>>>> + 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);
>>>>>>>>> @@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
>>>>>>>>> }
>>>>>>>>> EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>>>>>>>>
>>>>>>>>> +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;
>>>>>>>>> +
>>>>>>>>> + init_timer(&p->em485->stop_tx_timer);
>>>>>>>>> + p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
>>>>>>>>> + p->em485->stop_tx_timer.data = (unsigned long)p;
>>>>>>>>> + p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
>>>>>>>>
>>>>>>>> Not sure this is going to fly; this would be the only user of TIMER_IRQSAFE
>>>>>>>> (which was specifically introduced to workaround workqueue issues and not
>>>>>>>> meant for general use).
>>>>>>>
>>>>>>> This is required to call del_timer_sync(&p->em485->start_tx_timer);
>>>>>>> from __stop_tx_rs485
>>>>>>
>>>>>> I know; that doesn't mean it's ok.
>>>>>>
>>>>>
>>>>> What do you suggest? Run __stop_tx as a tasklet? I am not sure whether
>>>>> it introduces races or not.
>>>>
>>>> Would it be fine to use workqueues instead of timers? I mean
>>>> schedule_delayed_work and cancel_delayed_work_sync.
>>>> They use same timers with TIMER_IRQSAFE under the hood.
>>>> Or it is better to allocate separate work queue in order to achieve
>>>> better latency than shared system wq can provide?
>>>
>>> I think just del_timer() and locking with the port lock should be
>>> sufficient; timer + irq handler is nothing new.
>>>
>>
>> Do I understand correctly, that internals of
>> serial8250_em485_handle_stop_tx and serial8250_em485_handle_start_tx
>> should be wrapped with port->lock in order to ensure that they are not
>> running during the call going to run del_timer?
>
> Yes.
>
> Of course, you'll need some state mechanism to know in the timer function
> that the timer was cancelled. For example, in this situation
Sure, that's why I asked.
I've looked through the timer implementation, and found that I need an
additional variable to keep the state.
The running timer is indistinguishable from deleted one. Initially, I
hoped to check corresponding timer_list variable from timer function,
but this would not work.
>
> CPU 0 CPU 1
>
> start_tx_rs485() [timer fires]
> del_timer(stop_tx_timer)
> handle_stop_tx()
> spin_lock_irqsave(port lock)
> *waits*
> rts_on_send()
> mod_timer(start_tx_timer)
> *claims port lock*
>
> * obviously would be bad if *
> * do_stop_tx_rs485() ran now *
>
>
>
>>>>>>>>> + init_timer(&p->em485->start_tx_timer);
>>>>>>>>> + p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
>>>>>>>>> + p->em485->start_tx_timer.data = (unsigned long)p;
>>>>>>>>> + p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
>>>>>>>>> +
>>>>>>>>> + serial8250_em485_rts_after_send(p);
>>>>>>>>> +
>>>>>>>>> + return 0;
>>>>>>>>> +}
>>>>>>>>> +EXPORT_SYMBOL_GPL(serial8250_em485_init);
>>>>>>>>
>>>>>>>> Newline.
>>>>>>>>
>>>>>>>>> +void serial8250_em485_destroy(struct uart_8250_port *p)
>>>>>>>>> +{
>>>>>>>>> + if (p->em485 == NULL)
>>>>>>>>> + return;
>>>>>>>>> +
>>>>>>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>>>>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>>>>>>
>>>>>>>> What keeps start_tx() from restarting a new timer right here?
>>>>>>>
>>>>>>> Both start_tx and rs485_config (which calls destroy) are wrapped with
>>>>>>> port->lock in serial_core.c
>>>>>>
>>>>>> Ahh, missed that.
>>>>>>
>>>>>> Maybe it would be better simply to implement the config_rs485()
>>>>>> generically, and just call it from the omap_8250 config_rs485().
>>>>>>
>>>>>> And put a note about the locking in a function comment header
>>>>>>
>>>>>> /**
>>>>>> * serial8250_config_em485() - rs485 config helper
>>>>>> *
>>>>>> * ....
>>>>>> */
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>> + 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
>>>>>>>>> @@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
>>>>>>>>> serial8250_rpm_put(up);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> -static inline void __stop_tx(struct uart_8250_port *p)
>>>>>>>>> +static __u32 __start_tx_rs485(struct uart_8250_port *p)
>>>>>>>> ^^^^^
>>>>>>>> No need to preserve the userspace type here.
>>>>>>>>
>>>>>>>> The double underline leader in an identifier is typically used to distinguish
>>>>>>>> an unlocked version from a locked version. I don't think it's necessary here
>>>>>>>> or any of the other newly-introduced functions.
>>>>>>>
>>>>>>> I use double __ for consistency with __start_tx. Now I have:
>>>>>>>
>>>>>>> if (up->em485)
>>>>>>> __start_tx_rs485(port);
>>>>>>> else
>>>>>>> __start_tx(port);
>>>>>>
>>>>>> But __start_tx() is labelled that way to differentiate it from being identified
>>>>>> as the start_tx() handler (which is serial8250_start_tx()). IOW, contributors
>>>>>> unfamiliar with the 8250 driver itself won't become confused when grepping
>>>>>> for start_tx (or at least the idea is to minimize that confusion).
>>>>>>
>>>>>> start_tx_rs485() doesn't need differentiation, so doesn't require the
>>>>>> double __ leader.
>>>>>>
>>>>>> FWIW, this is consistent and typical elsewhere in the kernel.
>>>>>>
>>>>>>
>>>>>>>>> +{
>>>>>>>>> + if (!serial8250_em485_enabled(p))
>>>>>>>>> + return 0;
>>>>>>>>
>>>>>>>> Already checked that em485 was enabled in lone caller.
>>>>>>>>
>>>>>>>>
>>>>>>>>> + if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>>>>>>>>> + serial8250_stop_rx(&p->port);
>>>>>>>>> +
>>>>>>>>> + del_timer_sync(&p->em485->stop_tx_timer);
>>>>>>>>> +
>>>>>>>>> + if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
>>>>>>>>
>>>>>>>> Line too long. And just one negation is sufficient, ie.
>>>>>>>>
>>>>>>>> if (!(....) !=
>>>>>>>> !(....)) {
>>>>>>>>
>>>>>>>
>>>>>>> I would like to keep the double negation, in my opinion it is more
>>>>>>> clear to the reader and I believe that the compiler is able to
>>>>>>> optimize it.
>>>>>>>
>>>>>>>>> + serial8250_em485_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)
>>>>>>>>
>>>>>>>> Does this really need to be inline?
>>>>>>>>
>>>>>>>
>>>>>>> Why not?
>>>>>>
>>>>>> The expected yardstick for inline is some demonstrable speed improvement;
>>>>>> otherwise, size is favored.
>>>>>>
>>>>>> And __stop_tx() is already inlined in 3 places, which really doesn't
>>>>>> need inlining either -- a call/ret is nothing compared to device i/o.
>>>>>>
>>>>>
>>>>> Ok then, probably I am biased with my C++ experience and I am used to
>>>>> think that compiler considers `inline` only as a hint.
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Peter Hurley
>>>>>>
>>>>>>>>> +{
>>>>>>>>> + if (!serial8250_em485_enabled(p))
>>>>>>>>> + return;
>>>>>>>>> +
>>>>>>>>> + serial8250_em485_rts_after_send(p);
>>>>>>>>> + /*
>>>>>>>>> + * Empty the RX FIFO, we are not interested in anything
>>>>>>>>> + * received during the half-duplex transmission.
>>>>>>>>> + */
>>>>>>>>
>>>>>>>> Malformed block comment.
>>>>>>>>
>>>>>>>> /*
>>>>>>>> *
>>>>>>>> *
>>>>>>>> */
>>>>>>>>
>>>>>>>>> + 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;
>>>>>>>>> +
>>>>>>>>> + __do_stop_tx_rs485(p);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static inline void __stop_tx_rs485(struct uart_8250_port *p)
>>>>>>>>
>>>>>>>> Single caller so drop inline.
>>>>>>>>
>>>>>>>>> +{
>>>>>>>>> + if (!serial8250_em485_enabled(p))
>>>>>>>>> + return;
>>>>>>>>> +
>>>>>>>>> + del_timer_sync(&p->em485->start_tx_timer);
>>>>>>>>> +
>>>>>>>>> + /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */
>>>>>>>>
>>>>>>>> Block comment.
>>>>>>>>
>>>>>>>>> + if (p->port.rs485.delay_rts_after_send > 0) {
>>>>>>>>> + mod_timer(&p->em485->stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);
>>>>>>>>
>>>>>>>> Line too long; please re-format.
>>>>>>>> This is one problem with overly long identifiers.
>>>>>>>>
>>>>>>>>> + } 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;
>>>>>>>>> @@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p)
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> +static inline void __stop_tx(struct uart_8250_port *p)
>>>>>>>>> +{
>>>>>>>>> + if (serial8250_em485_enabled(p)) {
>>>>>>>>> + 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.
>>>>>>>>> + */
>>>>>>>>
>>>>>>>> Block indent.
>>>>>>>>
>>>>>>>> This code path should cancel start timer also.
>>>>>>>>
>>>>>>>>> + if (!((lsr & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
>>>>>>>>
>>>>>>>> if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>>>>>>>>
>>>>>>>>> + return;
>>>>>>>>> + }
>>>>>>>>> + __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);
>>>>>>>>> @@ -1319,12 +1450,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;
>>>>>>>>>
>>>>>>>>> @@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port)
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> +static void serial8250_em485_handle_start_tx(unsigned long arg)
>>>>>>>>> +{
>>>>>>>>> + struct uart_8250_port *p = (struct uart_8250_port *)arg;
>>>>>>>>> +
>>>>>>>>> + __start_tx(&p->port);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static void serial8250_start_tx(struct uart_port *port)
>>>>>>>>> +{
>>>>>>>>> + struct uart_8250_port *up = up_to_u8250p(port);
>>>>>>>>> + __u32 delay;
>>>>>>>>
>>>>>>>> int delay;
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> + serial8250_rpm_get_tx(up);
>>>>>>>>> +
>>>>>>>>> + if (up->em485 && timer_pending(&up->em485->start_tx_timer))
>>>>>>>>> + return;
>>>>>>>>> +
>>>>>>>>> + if (up->em485 && (delay = __start_tx_rs485(up))) {
>>>>>>>>
>>>>>>>> No assignment in conditional please.
>>>>>>>>
>>>>>>>>> + mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
>>>>>>>>> + } else {
>>>>>>>>> + __start_tx(port);
>>>>>>>>> + }
>>>>>>>>
>>>>>>>> Generally, braces aren't used for single statement if..else.
>>>>>>>> That probably won't apply here after removing the assignment-in-conditional,
>>>>>>>> but I thought it worth mentioning just so you know.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Peter Hurley
>>>>>>>>
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> 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..71516ec 100644
>>>>>>>>> --- a/include/linux/serial_8250.h
>>>>>>>>> +++ b/include/linux/serial_8250.h
>>>>>>>>> @@ -76,6 +76,11 @@ 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 */
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>> /*
>>>>>>>>> * This should be used by drivers which want to register
>>>>>>>>> * their own 8250 ports without registering their own
>>>>>>>>> @@ -122,6 +127,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)
>>>>>>>>>
>>>
>>
>>
>>
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-01-16 20:28 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-21 18:26 [PATCH v6 0/3] tty: Introduce software RS485 direction control support Matwey V. Kornilov
2015-12-21 18:26 ` [PATCH v6 1/3] tty: Move serial8250_stop_rx in front of serial8250_start_tx Matwey V. Kornilov
2015-12-21 18:26 ` [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 Matwey V. Kornilov
2016-01-15 16:14 ` Peter Hurley
2016-01-15 16:40 ` Peter Hurley
2016-01-15 18:42 ` Matwey V. Kornilov
2016-01-15 19:45 ` Peter Hurley
2016-01-15 20:01 ` Matwey V. Kornilov
2016-01-15 21:16 ` Matwey V. Kornilov
2016-01-15 22:17 ` Peter Hurley
2016-01-16 8:12 ` Matwey V. Kornilov
2016-01-16 18:56 ` Peter Hurley
2016-01-16 20:28 ` Matwey V. Kornilov
2016-01-15 20:20 ` Matwey V. Kornilov
2016-01-15 21:54 ` Peter Hurley
2015-12-21 18:26 ` [PATCH v6 3/3] tty: 8250_omap: Use software emulated RS485 direction control Matwey V. Kornilov
2016-01-15 16:32 ` Peter Hurley
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).