USB: serial: f81232: clear overrun flag
Subsystems (2)
PATCH v4 1/3 USB: serial: f81232: clear overrun flag
The F81232 will report data and LSR with bulk like following format:
bulk-in data: [LSR(1Byte)+DATA(1Byte)][LSR(1Byte)+DATA(1Byte)]...
LSR will auto clear frame/parity/break error flag when reading by H/W, but overrrun will only cleared when reading LSR. So this patch add a worker to read LSR when overrun and flush the worker on close() & suspend().
---
v4:
1: Add serial->suspending check in f81232_lsr_worker() to avoid re-trigger 2: Add port_priv-lsr_work_resched to re-trigger LSR worker
v3:
1: Add flush_work(&port_priv->lsr_work) in f81232_suspend().
v2:
1: Add flush_work(&port_priv->lsr_work) in f81232_close().
drivers/usb/serial/f81232.c | 57 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)
drivers/usb/serial/f81232.c +57 -0
@@ -41,12 +41,15 @@ MODULE_DEVICE_TABLE(usb, id_table);#define FIFO_CONTROL_REGISTER (0x02 + SERIAL_BASE_ADDRESS)#define LINE_CONTROL_REGISTER (0x03 + SERIAL_BASE_ADDRESS)#define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS) +#define LINE_STATUS_REGISTER (0x05 + SERIAL_BASE_ADDRESS)#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)struct f81232_private {struct mutex lock;u8 modem_control;u8 modem_status; +bool lsr_work_resched; +struct work_struct lsr_work;struct work_struct interrupt_work;struct usb_serial_port *port; };
@@ -282,6 +285,7 @@ static void f81232_read_int_callback(struct urb *urb)static void f81232_process_read_urb (struct urb *urb) {struct usb_serial_port *port = urb->context; +struct f81232_private *priv =usb_get_serial_port_data (port);unsigned char *data = urb->transfer_buffer;char tty_flag;unsigned int i;
@@ -315,6 +319,7 @@ static void f81232_process_read_urb(struct urb *urb)if (lsr &UART_LSR_OE ) { port->icount.overrun++; +schedule_work (&priv->lsr_work);tty_insert_flip_char (&port->port,0 ,TTY_OVERRUN ); }
@@ -556,9 +561,12 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)static void f81232_close (struct usb_serial_port *port) { +struct f81232_private *port_priv =usb_get_serial_port_data (port); +f81232_port_disable (port);usb_serial_generic_close (port);usb_kill_urb (port->interrupt_in_urb); +flush_work (&port_priv->lsr_work); }static void f81232_dtr_rts (struct usb_serial_port *port,int on)
@@ -603,6 +611,28 @@ static void f81232_interrupt_work(struct work_struct *work)f81232_read_msr (priv->port); } +static void f81232_lsr_worker (struct work_struct *work) +{ +struct f81232_private *priv; +struct usb_serial_port *port; +struct usb_serial *serial; +int status; +u8 tmp; + + priv = container_of(work, struct f81232_private, lsr_work); + port = priv->port; + serial = port->serial; + +if (serial->suspending) { + priv->lsr_work_resched = true; +return ; + } + + status =f81232_get_register (port,LINE_STATUS_REGISTER , &tmp); +if (status) +dev_warn (&port->dev,"read LSR failed: %d\n" , status); +} +static int f81232_port_probe (struct usb_serial_port *port) {struct f81232_private *priv;
@@ -613,6 +643,7 @@ static int f81232_port_probe(struct usb_serial_port *port)mutex_init (&priv->lock);INIT_WORK (&priv->interrupt_work, f81232_interrupt_work); +INIT_WORK (&priv->lsr_work, f81232_lsr_worker);usb_set_serial_port_data (port, priv);
@@ -632,6 +663,30 @@ static int f81232_port_remove(struct usb_serial_port *port)return 0 ; } +static int f81232_suspend (struct usb_serial *serial,pm_message_t message) +{ +struct f81232_private *port_priv; + + port_priv =usb_get_serial_port_data (serial->port[0 ]); +flush_work (&port_priv->lsr_work); + +return 0 ; +} + +static int f81232_resume (struct usb_serial *serial) +{ +struct f81232_private *port_priv; + + port_priv =usb_get_serial_port_data (serial->port[0 ]); + +if (port_priv->lsr_work_resched) { + port_priv->lsr_work_resched = false; +schedule_work (&port_priv->lsr_work); + } + +return usb_serial_generic_resume (serial); +} +static struct usb_serial_driver f81232_device = { .driver = { .owner =THIS_MODULE ,
@@ -655,6 +710,8 @@ static struct usb_serial_driver f81232_device = { .read_int_callback = f81232_read_int_callback, .port_probe = f81232_port_probe, .port_remove = f81232_port_remove, + .suspend = f81232_suspend, + .resume = f81232_resume, };static struct usb_serial_driver *const serial_drivers[] = {
PATCH v4 2/3 USB: serial: f81232: add high baud rate support
The F81232 had 4 clocksource 1.846/18.46/14.77/24MHz and baud rates can be up to 1.5Mbits with 24MHz.
F81232 Clock registers (106h)
Bit1-0: Clock source selector
00: 1.846MHz.
01: 18.46MHz.
10: 24MHz.
11: 14.77MHz.---
v4:
1: no change
v3:
1: change the baud_base in f81232_get_serial_info()
drivers/usb/serial/f81232.c | 105 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 94 insertions(+), 11 deletions(-)
drivers/usb/serial/f81232.c +94 -11
@@ -28,7 +28,8 @@ static const struct usb_device_id id_table[] = {MODULE_DEVICE_TABLE (usb, id_table);/* Maximum baudrate for F81232 */ -#define F81232_MAX_BAUDRATE 115200 +#define F81232_MAX_BAUDRATE 1500000 +#define F81232_DEF_BAUDRATE 9600/* USB Control EP parameter */ #define F81232_REGISTER_REQUEST 0xa0
@@ -44,19 +45,43 @@ MODULE_DEVICE_TABLE(usb, id_table);#define LINE_STATUS_REGISTER (0x05 + SERIAL_BASE_ADDRESS)#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS) +/* +* F81232 Clock registers (106h) +* +* Bit1-0: Clock source selector +* 00: 1.846MHz. +* 01: 18.46MHz. +* 10: 24MHz. +* 11: 14.77MHz. +*/ +#define F81232_CLK_REGISTER 0x106 +#define F81232_CLK_1_846_MHZ 0 +#define F81232_CLK_18_46_MHZ BIT(0) +#define F81232_CLK_24_MHZ BIT(1) +#define F81232_CLK_14_77_MHZ (BIT(1) | BIT(0)) +#define F81232_CLK_MASK GENMASK(1, 0) +struct f81232_private {struct mutex lock;u8 modem_control;u8 modem_status; +speed_t baud_base;bool lsr_work_resched;struct work_struct lsr_work;struct work_struct interrupt_work;struct usb_serial_port *port; }; -static int calc_baud_divisor (speed_t baudrate) +static u32 const baudrate_table[] = {115200 ,921600 ,1152000 ,1500000 }; +static u8 const clock_table[] = {F81232_CLK_1_846_MHZ ,F81232_CLK_14_77_MHZ , +F81232_CLK_18_46_MHZ ,F81232_CLK_24_MHZ }; + +static int calc_baud_divisor (speed_t baudrate,speed_t clockrate) { -return DIV_ROUND_CLOSEST (F81232_MAX_BAUDRATE , baudrate); +if (!baudrate) +return 0 ; + +return DIV_ROUND_CLOSEST (clockrate, baudrate); }static int f81232_get_register (struct usb_serial_port *port,u16 reg,u8 *val)
@@ -130,6 +155,21 @@ static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 val)return status; } +static int f81232_set_mask_register (struct usb_serial_port *port,u16 reg, +u8 mask,u8 val) +{ +int status; +u8 tmp; + + status =f81232_get_register (port, reg, &tmp); +if (status) +return status; + + tmp = (tmp & ~mask) | (val & mask); + +return f81232_set_register (port, reg, tmp); +} +static void f81232_read_msr (struct usb_serial_port *port) {int status;
@@ -347,13 +387,53 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)*/ } -static void f81232_set_baudrate (struct usb_serial_port *port,speed_t baudrate) +static int f81232_find_clk (speed_t baudrate) +{ +int idx; + +for (idx =0 ; idx <ARRAY_SIZE (baudrate_table); ++idx) { +if (baudrate <= baudrate_table[idx] && + baudrate_table[idx] % baudrate ==0 ) +return idx; + } + +return -EINVAL ; +} + +static void f81232_set_baudrate (struct tty_struct *tty, +struct usb_serial_port *port,speed_t baudrate, +speed_t old_baudrate) { +struct f81232_private *priv =usb_get_serial_port_data (port);u8 lcr;int divisor;int status =0 ; +int i; +int idx; +speed_t baud_list[] = {baudrate, old_baudrate,F81232_DEF_BAUDRATE }; + +for (i =0 ; i <ARRAY_SIZE (baud_list); ++i) { + idx =f81232_find_clk (baud_list[i]); +if (idx >=0 ) { + baudrate = baud_list[i]; +tty_encode_baud_rate (tty, baudrate, baudrate); +break ; + } + } - divisor =calc_baud_divisor (baudrate); +if (idx <0 ) +return ; + + priv->baud_base = baudrate_table[idx]; + divisor =calc_baud_divisor (baudrate, priv->baud_base); + + status =f81232_set_mask_register (port,F81232_CLK_REGISTER , +F81232_CLK_MASK , clock_table[idx]); +if (status) { +dev_err (&port->dev,"%s failed to set CLK_REG: %d\n" , + __func__, status); +return ; + } status =f81232_get_register (port,LINE_CONTROL_REGISTER , &lcr);/* get LCR */
@@ -443,6 +523,7 @@ static void f81232_set_termios(struct tty_struct *tty,u8 new_lcr =0 ;int status =0 ;speed_t baudrate; +speed_t old_baud;/* Don't change anything if nothing has changed */ if (old_termios && !tty_termios_hw_change (&tty->termios, old_termios))
@@ -455,11 +536,12 @@ static void f81232_set_termios(struct tty_struct *tty, baudrate =tty_get_baud_rate (tty);if (baudrate >0 ) { -if (baudrate >F81232_MAX_BAUDRATE ) { - baudrate =F81232_MAX_BAUDRATE ; -tty_encode_baud_rate (tty, baudrate, baudrate); - } -f81232_set_baudrate (port, baudrate); +if (old_termios) + old_baud =tty_termios_baud_rate (old_termios); +else + old_baud =F81232_DEF_BAUDRATE ; + +f81232_set_baudrate (tty, port, baudrate, old_baud); }if (C_PARENB (tty)) {
@@ -595,11 +677,12 @@ static int f81232_get_serial_info(struct tty_struct *tty, struct serial_struct *ss) {struct usb_serial_port *port = tty->driver_data; +struct f81232_private *priv =usb_get_serial_port_data (port); ss->type =PORT_16550A ; ss->line = port->minor; ss->port = port->port_number; - ss->baud_base =F81232_MAX_BAUDRATE ; + ss->baud_base = priv->baud_base;return 0 ; }
PATCH v4 3/3 USB: serial: f81232: implement break control
In reply to Ji-Ze Hong (Peter Hong)Implement Fintek F81232 break on/off with LCR register. It's the same with 16550A LCR register layout.
---
v4:
1: no change
v3:
1: using shadow_lcr to save LCR in f81232_break_ctl()
drivers/usb/serial/f81232.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
drivers/usb/serial/f81232.c +23 -6
@@ -65,6 +65,7 @@ struct f81232_private {struct mutex lock;u8 modem_control;u8 modem_status; +u8 shadow_lcr;speed_t baud_base;bool lsr_work_resched;struct work_struct lsr_work;
@@ -378,13 +379,23 @@ static void f81232_process_read_urb(struct urb *urb)static void f81232_break_ctl (struct tty_struct *tty,int break_state) { -/* FIXME - Stubbed out for now */ +struct usb_serial_port *port = tty->driver_data; +struct f81232_private *priv =usb_get_serial_port_data (port); +int status; -/* -* break_state = -1 to turn on break, and 0 to turn off break -* see drivers/char/tty_io.c to see it used. -* last_set_data_urb_value NEVER has the break bit set in it. -*/ +mutex_lock (&priv->lock); + +if (break_state) + priv->shadow_lcr |=UART_LCR_SBC ; +else + priv->shadow_lcr &= ~UART_LCR_SBC ; + + status =f81232_set_register (port,LINE_CONTROL_REGISTER , + priv->shadow_lcr); +if (status) +dev_err (&port->dev,"set break failed: %d\n" , status); + +mutex_unlock (&priv->lock); }static int f81232_find_clk (speed_t baudrate)
@@ -520,6 +531,7 @@ static int f81232_port_disable(struct usb_serial_port *port)static void f81232_set_termios (struct tty_struct *tty,struct usb_serial_port *port,struct ktermios *old_termios) { +struct f81232_private *priv =usb_get_serial_port_data (port);u8 new_lcr =0 ;int status =0 ;speed_t baudrate;
@@ -573,11 +585,16 @@ static void f81232_set_termios(struct tty_struct *tty,break ; } +mutex_lock (&priv->lock); + + new_lcr |= (priv->shadow_lcr &UART_LCR_SBC ); status =f81232_set_register (port,LINE_CONTROL_REGISTER , new_lcr);if (status) {dev_err (&port->dev,"%s failed to set LCR: %d\n" , __func__, status); } + +mutex_unlock (&priv->lock); }static int f81232_tiocmget (struct tty_struct *tty)
Quoted 7 lines · Ji-Ze Hong (Peter Hong)
The F81232 will report data and LSR with bulk like following format:
bulk-in data: [LSR(1Byte)+DATA(1Byte)][LSR(1Byte)+DATA(1Byte)]...LSR will auto clear frame/parity/break error flag when reading by H/W, but overrrun will only cleared when reading LSR. So this patch add a worker to read LSR when overrun and flush the worker on
close()&suspend().
Hi,
I really hate doing this to you, but you are exchanging one race condition for another race. Exact explanation below.
drivers/usb/serial/f81232.c static void f81232_process_read_urb(struct urb *urb) · line 322
Quoted 5 lines
drivers/usb/serial/f81232.c +1
-0@@ -315,6 +319,7 @@ static void f81232_process_read_urb(struct urb *urb)if (lsr &UART_LSR_OE ) { port->icount.overrun++; +schedule_work (&priv->lsr_work);
Again you schedule the work. It may run anytime. The check you put into the work item needs to go here.
drivers/usb/serial/f81232.c static void f81232_interrupt_work(struct work_struct *work) · line 626
Quoted 13 lines
drivers/usb/serial/f81232.c +13
-0+static void f81232_lsr_worker (struct work_struct *work) +{ +struct f81232_private *priv; +struct usb_serial_port *port; +struct usb_serial *serial; +int status; +u8 tmp; + + priv = container_of(work, struct f81232_private, lsr_work); + port = priv->port; + serial = port->serial; + +if (serial->suspending) {
There is no locking. f81232_resume() can run here. This test
if (port_priv->lsr_work_resched) {
will evaluate to false
Quoted 21 lines
drivers/usb/serial/f81232.c +10
-0+ priv->lsr_work_resched = true; +return ; + } + + status =f81232_get_register (port,LINE_STATUS_REGISTER , &tmp); +if (status) +dev_warn (&port->dev,"read LSR failed: %d\n" , status); +} +static int f81232_port_probe (struct usb_serial_port *port) {struct f81232_private *priv;@@ -613,6 +643,7 @@ static int f81232_port_probe(struct usb_serial_port *port)mutex_init (&priv->lock);INIT_WORK (&priv->interrupt_work, f81232_interrupt_work); +INIT_WORK (&priv->lsr_work, f81232_lsr_worker);usb_set_serial_port_data (port, priv);
drivers/usb/serial/f81232.c static int f81232_port_remove(struct usb_serial_port *port) · line 671
Quoted 9 lines
drivers/usb/serial/f81232.c +6
-0@@ -632,6 +663,30 @@ static int f81232_port_remove(struct usb_serial_port *port)return 0 ; } +static int f81232_suspend (struct usb_serial *serial,pm_message_t message) +{ +struct f81232_private *port_priv; + + port_priv =usb_get_serial_port_data (serial->port[0 ]); +flush_work (&port_priv->lsr_work);
Strictly speaking useless.
drivers/usb/serial/f81232.c static int f81232_port_remove(struct usb_serial_port *port) · line 683
Quoted 12 lines
drivers/usb/serial/f81232.c +12
-0+ +return 0 ; +} + +static int f81232_resume (struct usb_serial *serial) +{ +struct f81232_private *port_priv; + + port_priv =usb_get_serial_port_data (serial->port[0 ]); + +if (port_priv->lsr_work_resched) { + port_priv->lsr_work_resched = false;
Strictly speaking even that is a race, as you have no guarantee that your work queue is run before the system is suspended again. You are in a task context. There is no reason to defer action.
drivers/usb/serial/f81232.c static int f81232_port_remove(struct usb_serial_port *port) · line 688
Quoted 5 lines
drivers/usb/serial/f81232.c +5
-0+schedule_work (&port_priv->lsr_work); + } + +return usb_serial_generic_resume (serial); +}
Regards
Oliver