[PATCH V2 1/5] usb: serial: f81534: add high baud rate support

[PATCH V2 1/5] usb: serial: f81534: add high baud rate support

[Ji-Ze Hong (Peter Hong)]

The F81532/534 had 4 clocksource 1.846/18.46/14.77/24MHz and baud rates
can be up to 1.5Mbits with 24MHz.

This device may generate data overrun when baud rate setting to 921600bps
or higher with old UART trigger level setting (8x14=112) with full
loading. We'll change trigger level from 8x14=112 to 8x8=64 to avoid data
overrun.

Also the read/write of EP0 will be affected by this patch. The worst case
of responding time is 20s when all serial port are full loading and trying
to access EP0, so we change EP0 timeout from 10 to 20s.

F81532/534 Clock register (offset +08h)

Bit0:	UART Enable (always on)
Bit2-1:	Clock source selector
			00: 1.846MHz.
			01: 18.46MHz.
			10: 24MHz.
			11: 14.77MHz.

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
v2:
	1: Add commit message for F81534_USB_TIMEOUT from 1000 to 2000 and
	   trigger level from UART_FCR_R_TRIG_11 to UART_FCR_TRIGGER_8.
	2: Separate UART Enable bit from clock sources.
	3: Add help function "f81534_find_clk()" to calculate baud rate and
	   clock source
	4: Add shadow clock register for future use. 

 drivers/usb/serial/f81534.c | 87 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 71 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
index e4573b4c8935..758ef0424164 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -41,6 +41,7 @@
 #define F81534_MODEM_CONTROL_REG	(0x04 + F81534_UART_BASE_ADDRESS)
 #define F81534_LINE_STATUS_REG		(0x05 + F81534_UART_BASE_ADDRESS)
 #define F81534_MODEM_STATUS_REG		(0x06 + F81534_UART_BASE_ADDRESS)
+#define F81534_CLOCK_REG		(0x08 + F81534_UART_BASE_ADDRESS)
 #define F81534_CONFIG1_REG		(0x09 + F81534_UART_BASE_ADDRESS)
 
 #define F81534_DEF_CONF_ADDRESS_START	0x3000
@@ -57,7 +58,7 @@
 
 /* Default URB timeout for USB operations */
 #define F81534_USB_MAX_RETRY		10
-#define F81534_USB_TIMEOUT		1000
+#define F81534_USB_TIMEOUT		2000
 #define F81534_SET_GET_REGISTER		0xA0
 
 #define F81534_NUM_PORT			4
@@ -96,7 +97,6 @@
 #define F81534_CMD_READ			0x03
 
 #define F81534_DEFAULT_BAUD_RATE	9600
-#define F81534_MAX_BAUDRATE		115200
 
 #define F81534_PORT_CONF_DISABLE_PORT	BIT(3)
 #define F81534_PORT_CONF_NOT_EXIST_PORT	BIT(7)
@@ -106,6 +106,23 @@
 #define F81534_1X_RXTRIGGER		0xc3
 #define F81534_8X_RXTRIGGER		0xcf
 
+/*
+ * F81532/534 Clock registers (offset +08h)
+ *
+ * Bit0:	UART Enable (always on)
+ * Bit2-1:	Clock source selector
+ *			00: 1.846MHz.
+ *			01: 18.46MHz.
+ *			10: 24MHz.
+ *			11: 14.77MHz.
+ */
+
+#define F81534_UART_EN			BIT(0)
+#define F81534_CLK_1_846_MHZ		F81534_UART_EN
+#define F81534_CLK_18_46_MHZ		(F81534_UART_EN | BIT(1))
+#define F81534_CLK_24_MHZ		(F81534_UART_EN | BIT(2))
+#define F81534_CLK_14_77_MHZ		(F81534_UART_EN | BIT(1) | BIT(2))
+
 static const struct usb_device_id f81534_id_table[] = {
 	{ USB_DEVICE(FINTEK_VENDOR_ID_1, FINTEK_DEVICE_ID) },
 	{ USB_DEVICE(FINTEK_VENDOR_ID_2, FINTEK_DEVICE_ID) },
@@ -129,12 +146,18 @@ struct f81534_port_private {
 	struct usb_serial_port *port;
 	unsigned long tx_empty;
 	spinlock_t msr_lock;
+	u32 baud_base;
 	u8 shadow_mcr;
 	u8 shadow_lcr;
 	u8 shadow_msr;
+	u8 shadow_clk;
 	u8 phy_num;
 };
 
+static u32 const baudrate_table[] = {115200, 921600, 1152000, 1500000};
+static u8 const clock_table[] = {F81534_CLK_1_846_MHZ, F81534_CLK_14_77_MHZ,
+				F81534_CLK_18_46_MHZ, F81534_CLK_24_MHZ};
+
 static int f81534_logic_to_phy_port(struct usb_serial *serial,
 					struct usb_serial_port *port)
 {
@@ -460,13 +483,48 @@ static u32 f81534_calc_baud_divisor(u32 baudrate, u32 clockrate)
 	return DIV_ROUND_CLOSEST(clockrate, baudrate);
 }
 
-static int f81534_set_port_config(struct usb_serial_port *port, u32 baudrate,
-					u8 lcr)
+static int f81534_find_clk(u32 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 int f81534_set_port_config(struct usb_serial_port *port,
+		struct tty_struct *tty, u32 baudrate, u32 old_baudrate, u8 lcr)
 {
 	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
 	u32 divisor;
 	int status;
+	int i;
+	int idx;
 	u8 value;
+	u32 baud_list[] = {baudrate, old_baudrate, F81534_DEFAULT_BAUD_RATE};
+
+	for (i = 0; i < ARRAY_SIZE(baud_list); ++i) {
+		idx = f81534_find_clk(baud_list[i]);
+		if (idx >= 0) {
+			baudrate = baud_list[i];
+			tty_encode_baud_rate(tty, baudrate, baudrate);
+			break;
+		}
+	}
+
+	port_priv->baud_base = baudrate_table[idx];
+	port_priv->shadow_clk = clock_table[idx];
+
+	status = f81534_set_port_register(port, F81534_CLOCK_REG,
+			port_priv->shadow_clk);
+	if (status) {
+		dev_err(&port->dev, "CLOCK_REG setting failed\n");
+		return status;
+	}
 
 	if (baudrate <= 1200)
 		value = F81534_1X_RXTRIGGER;	/* 128 FIFO & TL: 1x */
@@ -482,7 +540,7 @@ static int f81534_set_port_config(struct usb_serial_port *port, u32 baudrate,
 	if (baudrate <= 1200)
 		value = UART_FCR_TRIGGER_1 | UART_FCR_ENABLE_FIFO; /* TL: 1 */
 	else
-		value = UART_FCR_R_TRIG_11 | UART_FCR_ENABLE_FIFO; /* TL: 14 */
+		value = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO; /* TL: 8 */
 
 	status = f81534_set_port_register(port, F81534_FIFO_CONTROL_REG,
 						value);
@@ -491,7 +549,7 @@ static int f81534_set_port_config(struct usb_serial_port *port, u32 baudrate,
 		return status;
 	}
 
-	divisor = f81534_calc_baud_divisor(baudrate, F81534_MAX_BAUDRATE);
+	divisor = f81534_calc_baud_divisor(baudrate, baudrate_table[idx]);
 
 	mutex_lock(&port_priv->lcr_mutex);
 
@@ -741,6 +799,7 @@ static void f81534_set_termios(struct tty_struct *tty,
 	u8 new_lcr = 0;
 	int status;
 	u32 baud;
+	u32 old_baud;
 
 	if (C_BAUD(tty) == B0)
 		f81534_update_mctrl(port, 0, TIOCM_DTR | TIOCM_RTS);
@@ -780,18 +839,14 @@ static void f81534_set_termios(struct tty_struct *tty,
 	if (!baud)
 		return;
 
-	if (baud > F81534_MAX_BAUDRATE) {
-		if (old_termios)
-			baud = tty_termios_baud_rate(old_termios);
-		else
-			baud = F81534_DEFAULT_BAUD_RATE;
-
-		tty_encode_baud_rate(tty, baud, baud);
-	}
+	if (old_termios)
+		old_baud = tty_termios_baud_rate(old_termios);
+	else
+		old_baud = F81534_DEFAULT_BAUD_RATE;
 
 	dev_dbg(&port->dev, "%s: baud: %d\n", __func__, baud);
 
-	status = f81534_set_port_config(port, baud, new_lcr);
+	status = f81534_set_port_config(port, tty, baud, old_baud, new_lcr);
 	if (status < 0) {
 		dev_err(&port->dev, "%s: set port config failed: %d\n",
 				__func__, status);
@@ -947,7 +1002,7 @@ static int f81534_get_serial_info(struct usb_serial_port *port,
 	tmp.type = PORT_16550A;
 	tmp.port = port->port_number;
 	tmp.line = port->minor;
-	tmp.baud_base = F81534_MAX_BAUDRATE;
+	tmp.baud_base = port_priv->baud_base;
 
 	if (copy_to_user(retinfo, &tmp, sizeof(*retinfo)))
 		return -EFAULT;
-- 
2.7.4

[PATCH V2 2/5] usb: serial: f81534: add auto RTS direction support

[Ji-Ze Hong (Peter Hong)]

The F81532/534 had auto RTS direction support for RS485 mode.
We'll read it from internal Flash with address 0x2f01~0x2f04 for 4 ports.
There are 4 conditions below:
	0: F81534_PORT_CONF_RS232.
	1: F81534_PORT_CONF_RS485.
	2: value error, default to F81534_PORT_CONF_RS232.
	3: F81534_PORT_CONF_RS485_INVERT.

F81532/534 Clock register (offset +08h)

Bit0:	UART Enable (always on)
Bit2-1:	Clock source selector
			00: 1.846MHz.
			01: 18.46MHz.
			10: 24MHz.
			11: 14.77MHz.
Bit4:	Auto direction(RTS) control (RTS pin Low when TX)
Bit5:	Invert direction(RTS) when Bit4 enabled (RTS pin high when TX)

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
V2:
	1: Read the configure data from flash and save it to shadow clock
	   register.

 drivers/usb/serial/f81534.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
index 758ef0424164..8a778bc1d492 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -98,11 +98,16 @@
 
 #define F81534_DEFAULT_BAUD_RATE	9600
 
+#define F81534_PORT_CONF_RS232		0
+#define F81534_PORT_CONF_RS485		BIT(0)
+#define F81534_PORT_CONF_RS485_INVERT	(BIT(0) | BIT(1))
 #define F81534_PORT_CONF_DISABLE_PORT	BIT(3)
 #define F81534_PORT_CONF_NOT_EXIST_PORT	BIT(7)
 #define F81534_PORT_UNAVAILABLE		\
 	(F81534_PORT_CONF_DISABLE_PORT | F81534_PORT_CONF_NOT_EXIST_PORT)
 
+#define F81534_UART_MODE_MASK		(BIT(0) | BIT(1))
+
 #define F81534_1X_RXTRIGGER		0xc3
 #define F81534_8X_RXTRIGGER		0xcf
 
@@ -115,6 +120,8 @@
  *			01: 18.46MHz.
  *			10: 24MHz.
  *			11: 14.77MHz.
+ * Bit4:	Auto direction(RTS) control (RTS pin Low when TX)
+ * Bit5:	Invert direction(RTS) when Bit4 enabled (RTS pin high when TX)
  */
 
 #define F81534_UART_EN			BIT(0)
@@ -123,6 +130,9 @@
 #define F81534_CLK_24_MHZ		(F81534_UART_EN | BIT(2))
 #define F81534_CLK_14_77_MHZ		(F81534_UART_EN | BIT(1) | BIT(2))
 
+#define F81534_CLK_RS485_MODE		BIT(4)
+#define F81534_CLK_RS485_INVERT		BIT(5)
+
 static const struct usb_device_id f81534_id_table[] = {
 	{ USB_DEVICE(FINTEK_VENDOR_ID_1, FINTEK_DEVICE_ID) },
 	{ USB_DEVICE(FINTEK_VENDOR_ID_2, FINTEK_DEVICE_ID) },
@@ -517,7 +527,8 @@ static int f81534_set_port_config(struct usb_serial_port *port,
 	}
 
 	port_priv->baud_base = baudrate_table[idx];
-	port_priv->shadow_clk = clock_table[idx];
+	port_priv->shadow_clk &= ~F81534_CLK_14_77_MHZ;
+	port_priv->shadow_clk |= clock_table[idx];
 
 	status = f81534_set_port_register(port, F81534_CLOCK_REG,
 			port_priv->shadow_clk);
@@ -1269,9 +1280,12 @@ static void f81534_lsr_worker(struct work_struct *work)
 
 static int f81534_port_probe(struct usb_serial_port *port)
 {
+	struct f81534_serial_private *serial_priv;
 	struct f81534_port_private *port_priv;
 	int ret;
+	u8 value;
 
+	serial_priv = usb_get_serial_data(port->serial);
 	port_priv = devm_kzalloc(&port->dev, sizeof(*port_priv), GFP_KERNEL);
 	if (!port_priv)
 		return -ENOMEM;
@@ -1303,6 +1317,24 @@ static int f81534_port_probe(struct usb_serial_port *port)
 	if (ret)
 		return ret;
 
+	value = serial_priv->conf_data[port_priv->phy_num];
+	switch (value & F81534_UART_MODE_MASK) {
+	case F81534_PORT_CONF_RS485_INVERT:
+		port_priv->shadow_clk = F81534_CLK_RS485_MODE |
+					F81534_CLK_RS485_INVERT;
+		dev_info(&port->dev, "RS485 invert mode.\n");
+		break;
+	case F81534_PORT_CONF_RS485:
+		port_priv->shadow_clk = F81534_CLK_RS485_MODE;
+		dev_info(&port->dev, "RS485 mode.\n");
+		break;
+
+	default:
+	case F81534_PORT_CONF_RS232:
+		dev_info(&port->dev, "RS232 mode.\n");
+		break;
+	}
+
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH V2 2/5] usb: serial: f81534: add auto RTS direction support

[Johan Hovold]

On Thu, Jan 04, 2018 at 10:29:18AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 had auto RTS direction support for RS485 mode.
> We'll read it from internal Flash with address 0x2f01~0x2f04 for 4 ports.
> There are 4 conditions below:
> 	0: F81534_PORT_CONF_RS232.
> 	1: F81534_PORT_CONF_RS485.
> 	2: value error, default to F81534_PORT_CONF_RS232.
> 	3: F81534_PORT_CONF_RS485_INVERT.
> 
> F81532/534 Clock register (offset +08h)
> 
> Bit0:	UART Enable (always on)
> Bit2-1:	Clock source selector
> 			00: 1.846MHz.
> 			01: 18.46MHz.
> 			10: 24MHz.
> 			11: 14.77MHz.
> Bit4:	Auto direction(RTS) control (RTS pin Low when TX)
> Bit5:	Invert direction(RTS) when Bit4 enabled (RTS pin high when TX)
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
> V2:
> 	1: Read the configure data from flash and save it to shadow clock
> 	   register.
> 
>  drivers/usb/serial/f81534.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index 758ef0424164..8a778bc1d492 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -98,11 +98,16 @@
>  
>  #define F81534_DEFAULT_BAUD_RATE	9600
>  
> +#define F81534_PORT_CONF_RS232		0
> +#define F81534_PORT_CONF_RS485		BIT(0)
> +#define F81534_PORT_CONF_RS485_INVERT	(BIT(0) | BIT(1))
>  #define F81534_PORT_CONF_DISABLE_PORT	BIT(3)
>  #define F81534_PORT_CONF_NOT_EXIST_PORT	BIT(7)
>  #define F81534_PORT_UNAVAILABLE		\
>  	(F81534_PORT_CONF_DISABLE_PORT | F81534_PORT_CONF_NOT_EXIST_PORT)
>  
> +#define F81534_UART_MODE_MASK		(BIT(0) | BIT(1))

Use GENMASK()?

> +
>  #define F81534_1X_RXTRIGGER		0xc3
>  #define F81534_8X_RXTRIGGER		0xcf
>  
> @@ -115,6 +120,8 @@
>   *			01: 18.46MHz.
>   *			10: 24MHz.
>   *			11: 14.77MHz.
> + * Bit4:	Auto direction(RTS) control (RTS pin Low when TX)
> + * Bit5:	Invert direction(RTS) when Bit4 enabled (RTS pin high when TX)
>   */
>  
>  #define F81534_UART_EN			BIT(0)
> @@ -123,6 +130,9 @@
>  #define F81534_CLK_24_MHZ		(F81534_UART_EN | BIT(2))
>  #define F81534_CLK_14_77_MHZ		(F81534_UART_EN | BIT(1) | BIT(2))
>  
> +#define F81534_CLK_RS485_MODE		BIT(4)
> +#define F81534_CLK_RS485_INVERT		BIT(5)
> +
>  static const struct usb_device_id f81534_id_table[] = {
>  	{ USB_DEVICE(FINTEK_VENDOR_ID_1, FINTEK_DEVICE_ID) },
>  	{ USB_DEVICE(FINTEK_VENDOR_ID_2, FINTEK_DEVICE_ID) },
> @@ -517,7 +527,8 @@ static int f81534_set_port_config(struct usb_serial_port *port,
>  	}
>  
>  	port_priv->baud_base = baudrate_table[idx];
> -	port_priv->shadow_clk = clock_table[idx];
> +	port_priv->shadow_clk &= ~F81534_CLK_14_77_MHZ;

Add a dedicated mask define instead of using a clock value here. No need
to clear the enable bit for example (which shouldn't be part of the
clock value as I mentioned earlier).

> +	port_priv->shadow_clk |= clock_table[idx];
>  
>  	status = f81534_set_port_register(port, F81534_CLOCK_REG,
>  			port_priv->shadow_clk);
> @@ -1269,9 +1280,12 @@ static void f81534_lsr_worker(struct work_struct *work)
>  
>  static int f81534_port_probe(struct usb_serial_port *port)
>  {
> +	struct f81534_serial_private *serial_priv;
>  	struct f81534_port_private *port_priv;
>  	int ret;
> +	u8 value;
>  
> +	serial_priv = usb_get_serial_data(port->serial);
>  	port_priv = devm_kzalloc(&port->dev, sizeof(*port_priv), GFP_KERNEL);
>  	if (!port_priv)
>  		return -ENOMEM;
> @@ -1303,6 +1317,24 @@ static int f81534_port_probe(struct usb_serial_port *port)
>  	if (ret)
>  		return ret;
>  
> +	value = serial_priv->conf_data[port_priv->phy_num];
> +	switch (value & F81534_UART_MODE_MASK) {
> +	case F81534_PORT_CONF_RS485_INVERT:
> +		port_priv->shadow_clk = F81534_CLK_RS485_MODE |
> +					F81534_CLK_RS485_INVERT;
> +		dev_info(&port->dev, "RS485 invert mode.\n");
> +		break;
> +	case F81534_PORT_CONF_RS485:
> +		port_priv->shadow_clk = F81534_CLK_RS485_MODE;
> +		dev_info(&port->dev, "RS485 mode.\n");
> +		break;
> +
> +	default:
> +	case F81534_PORT_CONF_RS232:
> +		dev_info(&port->dev, "RS232 mode.\n");

Again, I think these dev_info should really be dev_dbg.

> +		break;
> +	}
> +
>  	return 0;
>  }

Johan

[PATCH V2 3/5] usb: serial: f81534: add output pin control

[Ji-Ze Hong (Peter Hong)]

The F81532/534 had 3 output pin (M0/SD, M1, M2) with open-drain mode to
control transceiver. We'll read it from internal Flash with address
0x2f05~0x2f08 for 4 ports. The value is range from 0 to 7. The M0/SD is
MSB of this value. For a examples, If read value is 6, we'll write M0/SD,
M1, M2 as 1, 1, 0.

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
V2:
	1: Fix for space between brace.
	2: Remain the old pin control method.

 drivers/usb/serial/f81534.c | 67 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
index 8a778bc1d492..7f175f39a171 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -52,6 +52,7 @@
 #define F81534_CUSTOM_NO_CUSTOM_DATA	0xff
 #define F81534_CUSTOM_VALID_TOKEN	0xf0
 #define F81534_CONF_OFFSET		1
+#define F81534_CONF_GPIO_OFFSET		4
 
 #define F81534_MAX_DATA_BLOCK		64
 #define F81534_MAX_BUS_RETRY		20
@@ -164,6 +165,23 @@ struct f81534_port_private {
 	u8 phy_num;
 };
 
+struct f81534_pin_data {
+	const u16 reg_addr;
+	const u16 reg_mask;
+};
+
+struct f81534_port_out_pin {
+	struct f81534_pin_data pin[3];
+};
+
+/* Pin output value for M2/M1/M0(SD) */
+static const struct f81534_port_out_pin f81534_port_out_pins[] = {
+	 { { {0x2ae8, BIT(7)}, {0x2a90, BIT(5)}, {0x2a90, BIT(4) } } },
+	 { { {0x2ae8, BIT(6)}, {0x2ae8, BIT(0)}, {0x2ae8, BIT(3) } } },
+	 { { {0x2a90, BIT(0)}, {0x2ae8, BIT(2)}, {0x2a80, BIT(6) } } },
+	 { { {0x2a90, BIT(3)}, {0x2a90, BIT(2)}, {0x2a90, BIT(1) } } },
+};
+
 static u32 const baudrate_table[] = {115200, 921600, 1152000, 1500000};
 static u8 const clock_table[] = {F81534_CLK_1_846_MHZ, F81534_CLK_14_77_MHZ,
 				F81534_CLK_18_46_MHZ, F81534_CLK_24_MHZ};
@@ -273,6 +291,22 @@ static int f81534_get_register(struct usb_serial *serial, u16 reg, u8 *data)
 	return status;
 }
 
+static int f81534_set_mask_register(struct usb_serial *serial, u16 reg,
+					u8 mask, u8 data)
+{
+	int status;
+	u8 tmp;
+
+	status = f81534_get_register(serial, reg, &tmp);
+	if (status)
+		return status;
+
+	tmp &= ~mask;
+	tmp |= (mask & data);
+
+	return f81534_set_register(serial, reg, tmp);
+}
+
 static int f81534_set_port_register(struct usb_serial_port *port, u16 reg,
 					u8 data)
 {
@@ -1278,6 +1312,37 @@ static void f81534_lsr_worker(struct work_struct *work)
 		dev_warn(&port->dev, "read LSR failed: %d\n", status);
 }
 
+static int f81534_set_port_output_pin(struct usb_serial_port *port)
+{
+	struct f81534_serial_private *serial_priv;
+	struct f81534_port_private *port_priv;
+	struct usb_serial *serial;
+	const struct f81534_port_out_pin *pins;
+	int status;
+	int i;
+	u8 value;
+	u8 idx;
+
+	serial = port->serial;
+	serial_priv = usb_get_serial_data(serial);
+	port_priv = usb_get_serial_port_data(port);
+
+	idx = F81534_CONF_GPIO_OFFSET + port_priv->phy_num;
+	value = serial_priv->conf_data[idx];
+	pins = &f81534_port_out_pins[port_priv->phy_num];
+
+	for (i = 0; i < ARRAY_SIZE(pins->pin); ++i) {
+		status = f81534_set_mask_register(serial,
+				pins->pin[i].reg_addr, pins->pin[i].reg_mask,
+				value & BIT(i) ? pins->pin[i].reg_mask : 0);
+		if (status)
+			return status;
+	}
+
+	dev_dbg(&port->dev, "Output pin (M0/M1/M2): %d\n", value);
+	return 0;
+}
+
 static int f81534_port_probe(struct usb_serial_port *port)
 {
 	struct f81534_serial_private *serial_priv;
@@ -1335,7 +1400,7 @@ static int f81534_port_probe(struct usb_serial_port *port)
 		break;
 	}
 
-	return 0;
+	return f81534_set_port_output_pin(port);
 }
 
 static int f81534_port_remove(struct usb_serial_port *port)
-- 
2.7.4

Re: [PATCH V2 3/5] usb: serial: f81534: add output pin control

[Johan Hovold]

On Thu, Jan 04, 2018 at 10:29:19AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 had 3 output pin (M0/SD, M1, M2) with open-drain mode to
> control transceiver. We'll read it from internal Flash with address
> 0x2f05~0x2f08 for 4 ports. The value is range from 0 to 7. The M0/SD is
> MSB of this value. For a examples, If read value is 6, we'll write M0/SD,
> M1, M2 as 1, 1, 0.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
> V2:
> 	1: Fix for space between brace.
> 	2: Remain the old pin control method.
> 
>  drivers/usb/serial/f81534.c | 67 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index 8a778bc1d492..7f175f39a171 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -52,6 +52,7 @@
>  #define F81534_CUSTOM_NO_CUSTOM_DATA	0xff
>  #define F81534_CUSTOM_VALID_TOKEN	0xf0
>  #define F81534_CONF_OFFSET		1
> +#define F81534_CONF_GPIO_OFFSET		4
>  
>  #define F81534_MAX_DATA_BLOCK		64
>  #define F81534_MAX_BUS_RETRY		20
> @@ -164,6 +165,23 @@ struct f81534_port_private {
>  	u8 phy_num;
>  };
>  
> +struct f81534_pin_data {
> +	const u16 reg_addr;
> +	const u16 reg_mask;

I asked in my previous review comments whether this mask should really
be u8?

> +};
> +
> +struct f81534_port_out_pin {
> +	struct f81534_pin_data pin[3];
> +};
> +
> +/* Pin output value for M2/M1/M0(SD) */
> +static const struct f81534_port_out_pin f81534_port_out_pins[] = {
> +	 { { {0x2ae8, BIT(7)}, {0x2a90, BIT(5)}, {0x2a90, BIT(4) } } },
> +	 { { {0x2ae8, BIT(6)}, {0x2ae8, BIT(0)}, {0x2ae8, BIT(3) } } },
> +	 { { {0x2a90, BIT(0)}, {0x2ae8, BIT(2)}, {0x2a80, BIT(6) } } },
> +	 { { {0x2a90, BIT(3)}, {0x2a90, BIT(2)}, {0x2a90, BIT(1) } } },

Nit picking, but you still don't use space consistently around { and }
above.

Johan

[PATCH V2 4/5] usb: serial: f81534: add H/W disable port support

[Ji-Ze Hong (Peter Hong)]

The F81532/534 can be disable port by manufacturer with
following H/W design.
    1: Connect DCD/DSR/CTS/RI pin to ground.
    2: Connect RX pin to ground.

In driver, we'll implements some detect method likes following:
    1: Read MSR.
    2: Turn MCR LOOP bit on, off and read LSR after delay with 60ms.
       It'll contain BREAK status in LSR.

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
V2:
	1: f81534_check_port_hw_disabled() change return type from int to bool.
	2: Add help function f81534_set_phy_port_register() /
	   f81534_get_phy_port_register() for f81534_check_port_hw_disabled()
	   to read register without port.
	3: Re-write f81534_calc_num_ports() & f81534_attach() to reduce the
	   f81534_check_port_hw_disabled() repeatedly called.

 drivers/usb/serial/f81534.c | 160 +++++++++++++++++++++++++++-----------------
 1 file changed, 99 insertions(+), 61 deletions(-)

diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
index 7f175f39a171..a4666171239a 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -307,6 +307,20 @@ static int f81534_set_mask_register(struct usb_serial *serial, u16 reg,
 	return f81534_set_register(serial, reg, tmp);
 }
 
+static int f81534_set_phy_port_register(struct usb_serial *serial, int phy,
+					u16 reg, u8 data)
+{
+	return f81534_set_register(serial, reg + F81534_UART_OFFSET * phy,
+					data);
+}
+
+static int f81534_get_phy_port_register(struct usb_serial *serial, int phy,
+					u16 reg, u8 *data)
+{
+	return f81534_get_register(serial, reg + F81534_UART_OFFSET * phy,
+					data);
+}
+
 static int f81534_set_port_register(struct usb_serial_port *port, u16 reg,
 					u8 data)
 {
@@ -730,6 +744,70 @@ static int f81534_find_config_idx(struct usb_serial *serial, u8 *index)
 }
 
 /*
+ * The F81532/534 will not report serial port to USB serial subsystem when
+ * H/W DCD/DSR/CTS/RI/RX pin connected to ground.
+ *
+ * To detect RX pin status, we'll enable MCR interal loopback, disable it and
+ * delayed for 60ms. It connected to ground If LSR register report UART_LSR_BI.
+ */
+static bool f81534_check_port_hw_disabled(struct usb_serial *serial, int phy)
+{
+	int status;
+	u8 old_mcr;
+	u8 msr;
+	u8 lsr;
+	u8 msr_mask;
+
+	msr_mask = UART_MSR_DCD | UART_MSR_RI | UART_MSR_DSR | UART_MSR_CTS;
+
+	status = f81534_get_phy_port_register(serial, phy,
+				F81534_MODEM_STATUS_REG, &msr);
+	if (status)
+		return false;
+
+	if ((msr & msr_mask) != msr_mask)
+		return false;
+
+	status = f81534_set_phy_port_register(serial, phy,
+				F81534_FIFO_CONTROL_REG, UART_FCR_ENABLE_FIFO |
+				UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
+	if (status)
+		return false;
+
+	status = f81534_get_phy_port_register(serial, phy,
+				F81534_MODEM_CONTROL_REG, &old_mcr);
+	if (status)
+		return false;
+
+	status = f81534_set_phy_port_register(serial, phy,
+				F81534_MODEM_CONTROL_REG, UART_MCR_LOOP);
+	if (status)
+		return false;
+
+	status = f81534_set_phy_port_register(serial, phy,
+				F81534_MODEM_CONTROL_REG, 0x0);
+	if (status)
+		return false;
+
+	msleep(60);
+
+	status = f81534_get_phy_port_register(serial, phy,
+				F81534_LINE_STATUS_REG, &lsr);
+	if (status)
+		return false;
+
+	status = f81534_set_phy_port_register(serial, phy,
+				F81534_MODEM_CONTROL_REG, old_mcr);
+	if (status)
+		return false;
+
+	if ((lsr & UART_LSR_BI) == UART_LSR_BI)
+		return true;
+
+	return false;
+}
+
+/*
  * We had 2 generation of F81532/534 IC. All has an internal storage.
  *
  * 1st is pure USB-to-TTL RS232 IC and designed for 4 ports only, no any
@@ -750,11 +828,10 @@ static int f81534_find_config_idx(struct usb_serial *serial, u8 *index)
 static int f81534_calc_num_ports(struct usb_serial *serial,
 					struct usb_serial_endpoints *epds)
 {
+	struct f81534_serial_private *serial_priv;
 	struct device *dev = &serial->interface->dev;
 	int size_bulk_in = usb_endpoint_maxp(epds->bulk_in[0]);
 	int size_bulk_out = usb_endpoint_maxp(epds->bulk_out[0]);
-	u8 setting[F81534_CUSTOM_DATA_SIZE];
-	u8 setting_idx;
 	u8 num_port = 0;
 	int status;
 	size_t i;
@@ -765,8 +842,15 @@ static int f81534_calc_num_ports(struct usb_serial *serial,
 		return -ENODEV;
 	}
 
+	serial_priv = devm_kzalloc(&serial->interface->dev,
+					sizeof(*serial_priv), GFP_KERNEL);
+	if (!serial_priv)
+		return -ENOMEM;
+
+	usb_set_serial_data(serial, serial_priv);
+
 	/* Check had custom setting */
-	status = f81534_find_config_idx(serial, &setting_idx);
+	status = f81534_find_config_idx(serial, &serial_priv->setting_idx);
 	if (status) {
 		dev_err(&serial->interface->dev, "%s: find idx failed: %d\n",
 				__func__, status);
@@ -777,11 +861,12 @@ static int f81534_calc_num_ports(struct usb_serial *serial,
 	 * We'll read custom data only when data available, otherwise we'll
 	 * read default value instead.
 	 */
-	if (setting_idx != F81534_CUSTOM_NO_CUSTOM_DATA) {
+	if (serial_priv->setting_idx != F81534_CUSTOM_NO_CUSTOM_DATA) {
 		status = f81534_read_flash(serial,
 						F81534_CUSTOM_ADDRESS_START +
 						F81534_CONF_OFFSET,
-						sizeof(setting), setting);
+						sizeof(serial_priv->conf_data),
+						serial_priv->conf_data);
 		if (status) {
 			dev_err(&serial->interface->dev,
 					"%s: get custom data failed: %d\n",
@@ -791,13 +876,13 @@ static int f81534_calc_num_ports(struct usb_serial *serial,
 
 		dev_dbg(&serial->interface->dev,
 				"%s: read config from block: %d\n", __func__,
-				setting_idx);
+				serial_priv->setting_idx);
 	} else {
 		/* Read default board setting */
 		status = f81534_read_flash(serial,
-				F81534_DEF_CONF_ADDRESS_START, F81534_NUM_PORT,
-				setting);
-
+				F81534_DEF_CONF_ADDRESS_START,
+				sizeof(serial_priv->conf_data),
+				serial_priv->conf_data);
 		if (status) {
 			dev_err(&serial->interface->dev,
 					"%s: read failed: %d\n", __func__,
@@ -811,7 +896,10 @@ static int f81534_calc_num_ports(struct usb_serial *serial,
 
 	/* New style, find all possible ports */
 	for (i = 0; i < F81534_NUM_PORT; ++i) {
-		if (setting[i] & F81534_PORT_UNAVAILABLE)
+		if (f81534_check_port_hw_disabled(serial, i))
+			serial_priv->conf_data[i] |= F81534_PORT_UNAVAILABLE;
+
+		if (serial_priv->conf_data[i] & F81534_PORT_UNAVAILABLE)
 			continue;
 
 		++num_port;
@@ -1228,61 +1316,11 @@ static int f81534_attach(struct usb_serial *serial)
 {
 	struct f81534_serial_private *serial_priv;
 	int index = 0;
-	int status;
 	int i;
 
-	serial_priv = devm_kzalloc(&serial->interface->dev,
-					sizeof(*serial_priv), GFP_KERNEL);
-	if (!serial_priv)
-		return -ENOMEM;
-
-	usb_set_serial_data(serial, serial_priv);
-
+	serial_priv = usb_get_serial_data(serial);
 	mutex_init(&serial_priv->urb_mutex);
 
-	/* Check had custom setting */
-	status = f81534_find_config_idx(serial, &serial_priv->setting_idx);
-	if (status) {
-		dev_err(&serial->interface->dev, "%s: find idx failed: %d\n",
-				__func__, status);
-		return status;
-	}
-
-	/*
-	 * We'll read custom data only when data available, otherwise we'll
-	 * read default value instead.
-	 */
-	if (serial_priv->setting_idx == F81534_CUSTOM_NO_CUSTOM_DATA) {
-		/*
-		 * The default configuration layout:
-		 *	byte 0/1/2/3: uart setting
-		 */
-		status = f81534_read_flash(serial,
-					F81534_DEF_CONF_ADDRESS_START,
-					F81534_DEF_CONF_SIZE,
-					serial_priv->conf_data);
-		if (status) {
-			dev_err(&serial->interface->dev,
-					"%s: read reserve data failed: %d\n",
-					__func__, status);
-			return status;
-		}
-	} else {
-		/* Only read 8 bytes for mode & GPIO */
-		status = f81534_read_flash(serial,
-						F81534_CUSTOM_ADDRESS_START +
-						F81534_CONF_OFFSET,
-						sizeof(serial_priv->conf_data),
-						serial_priv->conf_data);
-		if (status) {
-			dev_err(&serial->interface->dev,
-					"%s: idx: %d get data failed: %d\n",
-					__func__, serial_priv->setting_idx,
-					status);
-			return status;
-		}
-	}
-
 	/* Assign phy-to-logic mapping */
 	for (i = 0; i < F81534_NUM_PORT; ++i) {
 		if (serial_priv->conf_data[i] & F81534_PORT_UNAVAILABLE)
-- 
2.7.4

Re: [PATCH V2 4/5] usb: serial: f81534: add H/W disable port support

[Johan Hovold]

On Thu, Jan 04, 2018 at 10:29:20AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 can be disable port by manufacturer with
> following H/W design.
>     1: Connect DCD/DSR/CTS/RI pin to ground.
>     2: Connect RX pin to ground.
> 
> In driver, we'll implements some detect method likes following:
>     1: Read MSR.
>     2: Turn MCR LOOP bit on, off and read LSR after delay with 60ms.
>        It'll contain BREAK status in LSR.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
> V2:
> 	1: f81534_check_port_hw_disabled() change return type from int to bool.
> 	2: Add help function f81534_set_phy_port_register() /
> 	   f81534_get_phy_port_register() for f81534_check_port_hw_disabled()
> 	   to read register without port.
> 	3: Re-write f81534_calc_num_ports() & f81534_attach() to reduce the
> 	   f81534_check_port_hw_disabled() repeatedly called.

This looks good, but please split up the config-data-readout refactoring
and f81534_check_port_hw_disabled() changes in two patches.

Johan

[PATCH V2 5/5] usb: serial: f81534: fix tx error on some baud rate

[Ji-Ze Hong (Peter Hong)]

The F81532/534 had 4 clocksource 1.846/18.46/14.77/24MHz and baud rates
can be up to 1.5Mbits with 24MHz. But on some baud rate (384~500kps), the
TX side will send the data frame too close to treat frame error on RX
side. This patch will force all TX data frame with delay 1bit gap.

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
V2:
	1: First introduced in this series patches.

 drivers/usb/serial/f81534.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
index a4666171239a..513805eeae6a 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -130,6 +130,7 @@
 #define F81534_CLK_18_46_MHZ		(F81534_UART_EN | BIT(1))
 #define F81534_CLK_24_MHZ		(F81534_UART_EN | BIT(2))
 #define F81534_CLK_14_77_MHZ		(F81534_UART_EN | BIT(1) | BIT(2))
+#define F81534_CLK_TX_DELAY_1BIT	BIT(3)
 
 #define F81534_CLK_RS485_MODE		BIT(4)
 #define F81534_CLK_RS485_INVERT		BIT(5)
@@ -1438,6 +1439,11 @@ static int f81534_port_probe(struct usb_serial_port *port)
 		break;
 	}
 
+	/*
+	 * We'll make tx frame error when baud rate from 384~500kps. So we'll
+	 * delay all tx data frame with 1bit.
+	 */
+	port_priv->shadow_clk |= F81534_CLK_TX_DELAY_1BIT;
 	return f81534_set_port_output_pin(port);
 }
 
-- 
2.7.4

Re: [PATCH V2 5/5] usb: serial: f81534: fix tx error on some baud rate

[Johan Hovold]

On Thu, Jan 04, 2018 at 10:29:21AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 had 4 clocksource 1.846/18.46/14.77/24MHz and baud rates
> can be up to 1.5Mbits with 24MHz. But on some baud rate (384~500kps), the
> TX side will send the data frame too close to treat frame error on RX
> side. This patch will force all TX data frame with delay 1bit gap.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
> V2:
> 	1: First introduced in this series patches.
> 
>  drivers/usb/serial/f81534.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index a4666171239a..513805eeae6a 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -130,6 +130,7 @@
>  #define F81534_CLK_18_46_MHZ		(F81534_UART_EN | BIT(1))
>  #define F81534_CLK_24_MHZ		(F81534_UART_EN | BIT(2))
>  #define F81534_CLK_14_77_MHZ		(F81534_UART_EN | BIT(1) | BIT(2))
> +#define F81534_CLK_TX_DELAY_1BIT	BIT(3)
>  
>  #define F81534_CLK_RS485_MODE		BIT(4)
>  #define F81534_CLK_RS485_INVERT		BIT(5)
> @@ -1438,6 +1439,11 @@ static int f81534_port_probe(struct usb_serial_port *port)
>  		break;
>  	}
>  
> +	/*
> +	 * We'll make tx frame error when baud rate from 384~500kps. So we'll
> +	 * delay all tx data frame with 1bit.
> +	 */
> +	port_priv->shadow_clk |= F81534_CLK_TX_DELAY_1BIT;

You don't wan't to enable this only for the affected rates?

>  	return f81534_set_port_output_pin(port);
>  }

Johan

Re: [PATCH V2 5/5] usb: serial: f81534: fix tx error on some baud rate

[Ji-Ze Hong (Peter Hong)]

Hi Johan,

Johan Hovold 於 2018/1/9 下午 07:32 寫道:
> On Thu, Jan 04, 2018 at 10:29:21AM +0800, Ji-Ze Hong (Peter Hong) wrote:
>> +	/*
>> +	 * We'll make tx frame error when baud rate from 384~500kps. So we'll
>> +	 * delay all tx data frame with 1bit.
>> +	 */
>> +	port_priv->shadow_clk |= F81534_CLK_TX_DELAY_1BIT;
> 
> You don't wan't to enable this only for the affected rates?
> 

This bit will control all transmit TX frame always delay 1 bit
on enabled, but It'll transmit TX frame randomly delay 1 bit on
disabled.

We had tested it with BurnInTest to check the performance,
It'll not make the performance regression. So we'll directly add
it on all baud rate.

Thanks
-- 
With Best Regards,
Peter Hong

Re: [PATCH V2 5/5] usb: serial: f81534: fix tx error on some baud rate

[Johan Hovold]

On Wed, Jan 10, 2018 at 01:42:32PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Hi Johan,
> 
> Johan Hovold 於 2018/1/9 下午 07:32 寫道:
> > On Thu, Jan 04, 2018 at 10:29:21AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> >> +	/*
> >> +	 * We'll make tx frame error when baud rate from 384~500kps. So we'll
> >> +	 * delay all tx data frame with 1bit.
> >> +	 */
> >> +	port_priv->shadow_clk |= F81534_CLK_TX_DELAY_1BIT;
> > 
> > You don't wan't to enable this only for the affected rates?
> > 
> 
> This bit will control all transmit TX frame always delay 1 bit
> on enabled, but It'll transmit TX frame randomly delay 1 bit on
> disabled.
> 
> We had tested it with BurnInTest to check the performance,
> It'll not make the performance regression. So we'll directly add
> it on all baud rate.

Ok, thanks for confirming.

Johan

Re: [PATCH V2 1/5] usb: serial: f81534: add high baud rate support

[Johan Hovold]

On Thu, Jan 04, 2018 at 10:29:17AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 had 4 clocksource 1.846/18.46/14.77/24MHz and baud rates
> can be up to 1.5Mbits with 24MHz.
> 
> This device may generate data overrun when baud rate setting to 921600bps
> or higher with old UART trigger level setting (8x14=112) with full
> loading. We'll change trigger level from 8x14=112 to 8x8=64 to avoid data
> overrun.
> 
> Also the read/write of EP0 will be affected by this patch. The worst case
> of responding time is 20s when all serial port are full loading and trying
> to access EP0, so we change EP0 timeout from 10 to 20s.

Surely you meant 1 and 2 seconds respectively here? And if you have
indeed measured response times close to 2000 ms then perhaps you want to
add even more margin?

> F81532/534 Clock register (offset +08h)
> 
> Bit0:	UART Enable (always on)
> Bit2-1:	Clock source selector
> 			00: 1.846MHz.
> 			01: 18.46MHz.
> 			10: 24MHz.
> 			11: 14.77MHz.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
> v2:
> 	1: Add commit message for F81534_USB_TIMEOUT from 1000 to 2000 and
> 	   trigger level from UART_FCR_R_TRIG_11 to UART_FCR_TRIGGER_8.
> 	2: Separate UART Enable bit from clock sources.
> 	3: Add help function "f81534_find_clk()" to calculate baud rate and
> 	   clock source
> 	4: Add shadow clock register for future use. 
> 
>  drivers/usb/serial/f81534.c | 87 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 71 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index e4573b4c8935..758ef0424164 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -41,6 +41,7 @@
>  #define F81534_MODEM_CONTROL_REG	(0x04 + F81534_UART_BASE_ADDRESS)
>  #define F81534_LINE_STATUS_REG		(0x05 + F81534_UART_BASE_ADDRESS)
>  #define F81534_MODEM_STATUS_REG		(0x06 + F81534_UART_BASE_ADDRESS)
> +#define F81534_CLOCK_REG		(0x08 + F81534_UART_BASE_ADDRESS)
>  #define F81534_CONFIG1_REG		(0x09 + F81534_UART_BASE_ADDRESS)
>  
>  #define F81534_DEF_CONF_ADDRESS_START	0x3000
> @@ -57,7 +58,7 @@
>  
>  /* Default URB timeout for USB operations */
>  #define F81534_USB_MAX_RETRY		10
> -#define F81534_USB_TIMEOUT		1000
> +#define F81534_USB_TIMEOUT		2000
>  #define F81534_SET_GET_REGISTER		0xA0
>  
>  #define F81534_NUM_PORT			4
> @@ -96,7 +97,6 @@
>  #define F81534_CMD_READ			0x03
>  
>  #define F81534_DEFAULT_BAUD_RATE	9600
> -#define F81534_MAX_BAUDRATE		115200
>  
>  #define F81534_PORT_CONF_DISABLE_PORT	BIT(3)
>  #define F81534_PORT_CONF_NOT_EXIST_PORT	BIT(7)
> @@ -106,6 +106,23 @@
>  #define F81534_1X_RXTRIGGER		0xc3
>  #define F81534_8X_RXTRIGGER		0xcf
>  
> +/*
> + * F81532/534 Clock registers (offset +08h)
> + *
> + * Bit0:	UART Enable (always on)
> + * Bit2-1:	Clock source selector
> + *			00: 1.846MHz.
> + *			01: 18.46MHz.
> + *			10: 24MHz.
> + *			11: 14.77MHz.
> + */
> +
> +#define F81534_UART_EN			BIT(0)
> +#define F81534_CLK_1_846_MHZ		F81534_UART_EN
> +#define F81534_CLK_18_46_MHZ		(F81534_UART_EN | BIT(1))
> +#define F81534_CLK_24_MHZ		(F81534_UART_EN | BIT(2))
> +#define F81534_CLK_14_77_MHZ		(F81534_UART_EN | BIT(1) | BIT(2))

I meant that you should keep the UART_EN define separate from the CLK
values and explicitly include it when you update the register (or just
once when you first initialise the shadow register during probe).

> +
>  static const struct usb_device_id f81534_id_table[] = {
>  	{ USB_DEVICE(FINTEK_VENDOR_ID_1, FINTEK_DEVICE_ID) },
>  	{ USB_DEVICE(FINTEK_VENDOR_ID_2, FINTEK_DEVICE_ID) },
> @@ -129,12 +146,18 @@ struct f81534_port_private {
>  	struct usb_serial_port *port;
>  	unsigned long tx_empty;
>  	spinlock_t msr_lock;
> +	u32 baud_base;
>  	u8 shadow_mcr;
>  	u8 shadow_lcr;
>  	u8 shadow_msr;
> +	u8 shadow_clk;
>  	u8 phy_num;
>  };
>  
> +static u32 const baudrate_table[] = {115200, 921600, 1152000, 1500000};
> +static u8 const clock_table[] = {F81534_CLK_1_846_MHZ, F81534_CLK_14_77_MHZ,
> +				F81534_CLK_18_46_MHZ, F81534_CLK_24_MHZ};
> +
>  static int f81534_logic_to_phy_port(struct usb_serial *serial,
>  					struct usb_serial_port *port)
>  {
> @@ -460,13 +483,48 @@ static u32 f81534_calc_baud_divisor(u32 baudrate, u32 clockrate)
>  	return DIV_ROUND_CLOSEST(clockrate, baudrate);
>  }
>  
> -static int f81534_set_port_config(struct usb_serial_port *port, u32 baudrate,
> -					u8 lcr)
> +static int f81534_find_clk(u32 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 int f81534_set_port_config(struct usb_serial_port *port,
> +		struct tty_struct *tty, u32 baudrate, u32 old_baudrate, u8 lcr)
>  {
>  	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
>  	u32 divisor;
>  	int status;
> +	int i;
> +	int idx;
>  	u8 value;
> +	u32 baud_list[] = {baudrate, old_baudrate, F81534_DEFAULT_BAUD_RATE};
> +
> +	for (i = 0; i < ARRAY_SIZE(baud_list); ++i) {
> +		idx = f81534_find_clk(baud_list[i]);
> +		if (idx >= 0) {
> +			baudrate = baud_list[i];
> +			tty_encode_baud_rate(tty, baudrate, baudrate);
> +			break;
> +		}
> +	}

I know you will (currently) always find a valid baudrate above, but for
good measure add a sanity check on idx here.

> +
> +	port_priv->baud_base = baudrate_table[idx];
> +	port_priv->shadow_clk = clock_table[idx];
> +
> +	status = f81534_set_port_register(port, F81534_CLOCK_REG,
> +			port_priv->shadow_clk);
> +	if (status) {
> +		dev_err(&port->dev, "CLOCK_REG setting failed\n");
> +		return status;
> +	}
>  
>  	if (baudrate <= 1200)
>  		value = F81534_1X_RXTRIGGER;	/* 128 FIFO & TL: 1x */
> @@ -482,7 +540,7 @@ static int f81534_set_port_config(struct usb_serial_port *port, u32 baudrate,
>  	if (baudrate <= 1200)
>  		value = UART_FCR_TRIGGER_1 | UART_FCR_ENABLE_FIFO; /* TL: 1 */
>  	else
> -		value = UART_FCR_R_TRIG_11 | UART_FCR_ENABLE_FIFO; /* TL: 14 */
> +		value = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO; /* TL: 8 */
>  
>  	status = f81534_set_port_register(port, F81534_FIFO_CONTROL_REG,
>  						value);
> @@ -491,7 +549,7 @@ static int f81534_set_port_config(struct usb_serial_port *port, u32 baudrate,
>  		return status;
>  	}
>  
> -	divisor = f81534_calc_baud_divisor(baudrate, F81534_MAX_BAUDRATE);
> +	divisor = f81534_calc_baud_divisor(baudrate, baudrate_table[idx]);

Use priv->baud_base here instead of table[idx]?

Johan

Re: [PATCH V2 1/5] usb: serial: f81534: add high baud rate support

[Ji-Ze Hong (Peter Hong)]

Hi Johan,

Johan Hovold 於 2018/1/9 下午 07:08 寫道:
> On Thu, Jan 04, 2018 at 10:29:17AM +0800, Ji-Ze Hong (Peter Hong) wrote:
>> The F81532/534 had 4 clocksource 1.846/18.46/14.77/24MHz and baud rates
>> can be up to 1.5Mbits with 24MHz.
>>
>> This device may generate data overrun when baud rate setting to 921600bps
>> or higher with old UART trigger level setting (8x14=112) with full
>> loading. We'll change trigger level from 8x14=112 to 8x8=64 to avoid data
>> overrun.
>>
>> Also the read/write of EP0 will be affected by this patch. The worst case
>> of responding time is 20s when all serial port are full loading and trying
>> to access EP0, so we change EP0 timeout from 10 to 20s.
> 
> Surely you meant 1 and 2 seconds respectively here? And if you have
> indeed measured response times close to 2000 ms then perhaps you want to
> add even more margin?

Normally, the communication with F81534 ep0 will take less than 1 sec
(even only some milliseconds), but It maybe take much long time with
huge loading with UART functional.

We had tested it on BurnInTest, 4 ports with 921600bps + MSR status
check to perform huge loading test. The worst case to read MSR register
via ep0 will take 15~18 seconds. So We'll still remain the max waiting
time for access ep0 with 2x10=20s in high baud rate mode.

Thanks
-- 
With Best Regards,
Peter Hong

Re: [PATCH V2 1/5] usb: serial: f81534: add high baud rate support

[Johan Hovold]

On Wed, Jan 10, 2018 at 01:30:18PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Hi Johan,
> 
> Johan Hovold 於 2018/1/9 下午 07:08 寫道:
> > On Thu, Jan 04, 2018 at 10:29:17AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> >> The F81532/534 had 4 clocksource 1.846/18.46/14.77/24MHz and baud rates
> >> can be up to 1.5Mbits with 24MHz.
> >>
> >> This device may generate data overrun when baud rate setting to 921600bps
> >> or higher with old UART trigger level setting (8x14=112) with full
> >> loading. We'll change trigger level from 8x14=112 to 8x8=64 to avoid data
> >> overrun.
> >>
> >> Also the read/write of EP0 will be affected by this patch. The worst case
> >> of responding time is 20s when all serial port are full loading and trying
> >> to access EP0, so we change EP0 timeout from 10 to 20s.
> > 
> > Surely you meant 1 and 2 seconds respectively here? And if you have
> > indeed measured response times close to 2000 ms then perhaps you want to
> > add even more margin?
> 
> Normally, the communication with F81534 ep0 will take less than 1 sec
> (even only some milliseconds), but It maybe take much long time with
> huge loading with UART functional.
> 
> We had tested it on BurnInTest, 4 ports with 921600bps + MSR status
> check to perform huge loading test. The worst case to read MSR register
> via ep0 will take 15~18 seconds. So We'll still remain the max waiting
> time for access ep0 with 2x10=20s in high baud rate mode.

Wow, that's unfortunate. But note that your patch only doubles the
timeout to 2000 ms, that is, two seconds and not twenty:

-#define F81534_USB_TIMEOUT             1000
+#define F81534_USB_TIMEOUT             2000

If you really intended to increase this to twenty seconds, then please
do so in a separate (preparatory) patch where you describe why that is
needed (e.g. what you wrote above).

Johan

Re: [PATCH V2 1/5] usb: serial: f81534: add high baud rate support

[Ji-Ze Hong (Peter Hong)]

Hi Johan,

Johan Hovold 於 2018/1/10 下午 04:49 寫道:
>> Normally, the communication with F81534 ep0 will take less than 1 sec
>> (even only some milliseconds), but It maybe take much long time with
>> huge loading with UART functional.
>>
>> We had tested it on BurnInTest, 4 ports with 921600bps + MSR status
>> check to perform huge loading test. The worst case to read MSR register
>> via ep0 will take 15~18 seconds. So We'll still remain the max waiting
>> time for access ep0 with 2x10=20s in high baud rate mode.
> 
> Wow, that's unfortunate. But note that your patch only doubles the
> timeout to 2000 ms, that is, two seconds and not twenty:
> 
> -#define F81534_USB_TIMEOUT             1000
> +#define F81534_USB_TIMEOUT             2000
> 
> If you really intended to increase this to twenty seconds, then please
> do so in a separate (preparatory) patch where you describe why that is
> needed (e.g. what you wrote above).

In f81534_set_register()/f81534_get_register(), We'll use a while loop
with 10 times to get/set register and the timeout is 1000ms. So the
total minimum retry timeout is 1000x10=10s.

But when introducing the high baud rate support, 10s is not enough for
heavily loading. We had tested the minimum retry is 16~18s, so we
enlarge the F81534_USB_TIMEOUT from 1000 to 2000 and the total minimum
retry timeout is 20s.

Should I separate it as 2 patches? This issue is due to introducing
high baud patch.

Thanks
-- 
With Best Regards,
Peter Hong

Re: [PATCH V2 1/5] usb: serial: f81534: add high baud rate support

[Johan Hovold]

On Wed, Jan 10, 2018 at 05:16:01PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Hi Johan,
> 
> Johan Hovold 於 2018/1/10 下午 04:49 寫道:
> >> Normally, the communication with F81534 ep0 will take less than 1 sec
> >> (even only some milliseconds), but It maybe take much long time with
> >> huge loading with UART functional.
> >>
> >> We had tested it on BurnInTest, 4 ports with 921600bps + MSR status
> >> check to perform huge loading test. The worst case to read MSR register
> >> via ep0 will take 15~18 seconds. So We'll still remain the max waiting
> >> time for access ep0 with 2x10=20s in high baud rate mode.
> > 
> > Wow, that's unfortunate. But note that your patch only doubles the
> > timeout to 2000 ms, that is, two seconds and not twenty:
> > 
> > -#define F81534_USB_TIMEOUT             1000
> > +#define F81534_USB_TIMEOUT             2000
> > 
> > If you really intended to increase this to twenty seconds, then please
> > do so in a separate (preparatory) patch where you describe why that is
> > needed (e.g. what you wrote above).
> 
> In f81534_set_register()/f81534_get_register(), We'll use a while loop
> with 10 times to get/set register and the timeout is 1000ms. So the
> total minimum retry timeout is 1000x10=10s.
> 
> But when introducing the high baud rate support, 10s is not enough for
> heavily loading. We had tested the minimum retry is 16~18s, so we
> enlarge the F81534_USB_TIMEOUT from 1000 to 2000 and the total minimum
> retry timeout is 20s.
> 
> Should I separate it as 2 patches? This issue is due to introducing
> high baud patch.

Ah, sorry. Forgot about the retries. Increasing it as part of this patch
is fine.

Johan