* [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
* 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 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 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 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
* 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: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
* [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 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
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).