[PATCH V1 1/4] usb: serial: f81534: add high baud rate support

[PATCH V1 1/4] 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.

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>
---
 drivers/usb/serial/f81534.c | 84 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 68 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
index cb8214860192..76c676ef5f0d 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -45,6 +45,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
@@ -61,7 +62,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
@@ -100,7 +101,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)
@@ -110,6 +110,22 @@
 #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_CLK_1_846_MHZ		BIT(0)
+#define F81534_CLK_18_46_MHZ		(BIT(0) | BIT(1))
+#define F81534_CLK_24_MHZ		(BIT(0) | BIT(2))
+#define F81534_CLK_14_77_MHZ		(BIT(0) | 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) },
@@ -133,6 +149,7 @@ 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;
@@ -464,13 +481,51 @@ 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_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 idx;
 	u8 value;
+	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};
+
+	do {
+		for (idx = 0; idx < ARRAY_SIZE(baudrate_table); ++idx) {
+			if (baudrate <= baudrate_table[idx] &&
+					baudrate_table[idx] % baudrate == 0)
+				break;
+		}
+
+		/* Found acceptable baud rate */
+		if (idx != ARRAY_SIZE(baudrate_table))
+			break;
+
+		if (baudrate == old_baudrate &&
+				old_baudrate != F81534_DEFAULT_BAUD_RATE)
+			old_baudrate = F81534_DEFAULT_BAUD_RATE;
+
+		dev_warn(&port->dev,
+				"baudrate: %d not supported, change to: %d\n",
+				baudrate, old_baudrate);
+
+		baudrate = old_baudrate;
+		tty_encode_baud_rate(tty, baudrate, baudrate);
+
+	} while (1);
+
+	port_priv->baud_base = baudrate_table[idx];
+	status = f81534_set_port_register(port, F81534_CLOCK_REG,
+			clock_table[idx]);
+	if (status) {
+		dev_err(&port->dev, "CLOCK_REG setting failed\n");
+		return status;
+	}
 
 	if (baudrate <= 1200)
 		value = F81534_1X_RXTRIGGER;	/* 128 FIFO & TL: 1x */
@@ -486,7 +541,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);
@@ -495,7 +550,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);
 
@@ -745,6 +800,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);
@@ -784,18 +840,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);
@@ -951,7 +1003,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 V1 2/4] 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>
---
 drivers/usb/serial/f81534.c | 54 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
index 76c676ef5f0d..b2d10309c335 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -102,11 +102,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
 
@@ -119,6 +124,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_CLK_1_846_MHZ		BIT(0)
@@ -126,6 +133,9 @@
 #define F81534_CLK_24_MHZ		(BIT(0) | BIT(2))
 #define F81534_CLK_14_77_MHZ		(BIT(0) | 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) },
@@ -485,16 +495,20 @@ 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);
+	struct f81534_serial_private *serial_priv;
 	u32 divisor;
 	int status;
 	int idx;
 	u8 value;
+	u8 tmp;
 	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};
 
+	serial_priv = usb_get_serial_data(port->serial);
+
 	do {
 		for (idx = 0; idx < ARRAY_SIZE(baudrate_table); ++idx) {
 			if (baudrate <= baudrate_table[idx] &&
@@ -520,8 +534,25 @@ static int f81534_set_port_config(struct usb_serial_port *port,
 	} while (1);
 
 	port_priv->baud_base = baudrate_table[idx];
-	status = f81534_set_port_register(port, F81534_CLOCK_REG,
-			clock_table[idx]);
+	tmp = serial_priv->conf_data[port_priv->phy_num];
+
+	switch (tmp & F81534_UART_MODE_MASK) {
+	case F81534_PORT_CONF_RS485_INVERT:
+		value = F81534_CLK_RS485_MODE | F81534_CLK_RS485_INVERT;
+		break;
+	case F81534_PORT_CONF_RS485:
+		value = F81534_CLK_RS485_MODE;
+		break;
+
+	default:
+		/* fall through, default RS232 Mode */
+	case F81534_PORT_CONF_RS232:
+		value = 0;
+		break;
+	}
+
+	value |= clock_table[idx];
+	status = f81534_set_port_register(port, F81534_CLOCK_REG, value);
 	if (status) {
 		dev_err(&port->dev, "CLOCK_REG setting failed\n");
 		return status;
@@ -1270,9 +1301,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;
@@ -1304,6 +1338,22 @@ 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:
+		dev_info(&port->dev, "RS485 invert mode.\n");
+		break;
+	case F81534_PORT_CONF_RS485:
+		dev_info(&port->dev, "RS485 mode.\n");
+		break;
+
+	default:
+		/* fall through, default RS232 Mode */
+	case F81534_PORT_CONF_RS232:
+		dev_info(&port->dev, "RS232 mode.\n");
+		break;
+	}
+
 	return 0;
 }
 
-- 
2.7.4

[PATCH V1 3/4] 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.

Register mapping for output value:
	Port 0:
		M2: 0x2ae8 bit7, M1: 0x2a90 bit5, M0/SD: 0x2a90 bit4
	Port 1:
		M2: 0x2ae8 bit6, M1: 0x2ae8 bit0, M0/SD: 0x2ae8 bit3
	Port 2:
		M2: 0x2a90 bit0, M1: 0x2ae8 bit2, M0/SD: 0x2a80 bit6
	Port 3:
		M2: 0x2a90 bit3, M1: 0x2a90 bit2, M0/SD: 0x2a90 bit1

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
 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 b2d10309c335..30b966d71ae8 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -56,6 +56,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
@@ -166,6 +167,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 int f81534_logic_to_phy_port(struct usb_serial *serial,
 					struct usb_serial_port *port)
 {
@@ -271,6 +289,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)
 {
@@ -1299,6 +1333,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_info(&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;
@@ -1354,7 +1419,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

[PATCH V1 4/4] 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>
---
 drivers/usb/serial/f81534.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
index 30b966d71ae8..18bd2a478199 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -751,6 +751,74 @@ 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 int 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_register(serial,
+				F81534_MODEM_STATUS_REG + phy * 0x10, &msr);
+	if (status)
+		return status;
+
+	if ((msr & msr_mask) != msr_mask)
+		return 0;
+
+	status = f81534_set_register(serial,
+				F81534_FIFO_CONTROL_REG + phy * 0x10,
+				UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR |
+				UART_FCR_CLEAR_XMIT);
+	if (status)
+		return status;
+
+	status = f81534_get_register(serial,
+				F81534_MODEM_CONTROL_REG + phy * 0x10,
+				&old_mcr);
+	if (status)
+		return status;
+
+	status = f81534_set_register(serial,
+				F81534_MODEM_CONTROL_REG + phy * 0x10,
+				UART_MCR_LOOP);
+	if (status)
+		return status;
+
+	status = f81534_set_register(serial,
+				F81534_MODEM_CONTROL_REG + phy * 0x10, 0x0);
+	if (status)
+		return status;
+
+	msleep(60);
+
+	status = f81534_get_register(serial,
+				F81534_LINE_STATUS_REG + phy * 0x10, &lsr);
+	if (status)
+		return status;
+
+	status = f81534_set_register(serial,
+				F81534_MODEM_CONTROL_REG + phy * 0x10,
+				old_mcr);
+	if (status)
+		return status;
+
+	if ((lsr & UART_LSR_BI) == UART_LSR_BI)
+		return -ENODEV;
+
+	return 0;
+}
+
+/*
  * 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
@@ -832,6 +900,9 @@ 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 (f81534_check_port_hw_disabled(serial, i))
+			continue;
+
 		if (setting[i] & F81534_PORT_UNAVAILABLE)
 			continue;
 
@@ -1306,6 +1377,9 @@ static int f81534_attach(struct usb_serial *serial)
 
 	/* Assign phy-to-logic mapping */
 	for (i = 0; i < F81534_NUM_PORT; ++i) {
+		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;
 
-- 
2.7.4

Re: [PATCH V1 1/4] usb: serial: f81534: add high baud rate support

[Johan Hovold]

On Thu, Nov 16, 2017 at 03:46:06PM +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.
> 
> 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>
> ---
>  drivers/usb/serial/f81534.c | 84 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 68 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index cb8214860192..76c676ef5f0d 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -45,6 +45,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
> @@ -61,7 +62,7 @@
>  
>  /* Default URB timeout for USB operations */
>  #define F81534_USB_MAX_RETRY		10
> -#define F81534_USB_TIMEOUT		1000
> +#define F81534_USB_TIMEOUT		2000

You need to at least mention in the commit message why this is needed.

>  #define F81534_SET_GET_REGISTER		0xA0
>  
>  #define F81534_NUM_PORT			4
> @@ -100,7 +101,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)
> @@ -110,6 +110,22 @@
>  #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_CLK_1_846_MHZ		BIT(0)
> +#define F81534_CLK_18_46_MHZ		(BIT(0) | BIT(1))
> +#define F81534_CLK_24_MHZ		(BIT(0) | BIT(2))
> +#define F81534_CLK_14_77_MHZ		(BIT(0) | BIT(1) | BIT(2))

Use a separate define for the enable bit.

> +
>  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) },
> @@ -133,6 +149,7 @@ 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;
> @@ -464,13 +481,51 @@ 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_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 idx;
>  	u8 value;
> +	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};
> +
> +	do {
> +		for (idx = 0; idx < ARRAY_SIZE(baudrate_table); ++idx) {
> +			if (baudrate <= baudrate_table[idx] &&
> +					baudrate_table[idx] % baudrate == 0)
> +				break;
> +		}
> +
> +		/* Found acceptable baud rate */
> +		if (idx != ARRAY_SIZE(baudrate_table))
> +			break;
> +
> +		if (baudrate == old_baudrate &&
> +				old_baudrate != F81534_DEFAULT_BAUD_RATE)
> +			old_baudrate = F81534_DEFAULT_BAUD_RATE;
> +
> +		dev_warn(&port->dev,
> +				"baudrate: %d not supported, change to: %d\n",
> +				baudrate, old_baudrate);

No need to warn here as you report back the chosen speed in the termios.

> +
> +		baudrate = old_baudrate;
> +		tty_encode_baud_rate(tty, baudrate, baudrate);
> +
> +	} while (1);

This looks scary. Try to clean it up by adding helper function to
determine the base.

> +
> +	port_priv->baud_base = baudrate_table[idx];
> +	status = f81534_set_port_register(port, F81534_CLOCK_REG,
> +			clock_table[idx]);
> +	if (status) {
> +		dev_err(&port->dev, "CLOCK_REG setting failed\n");
> +		return status;
> +	}

I suggest you add a shadow clock register which keeps the enable bit
set. This should allow for a cleaner implementation of the auto-RTS
handling in later patches.

>  
>  	if (baudrate <= 1200)
>  		value = F81534_1X_RXTRIGGER;	/* 128 FIFO & TL: 1x */
> @@ -486,7 +541,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 */

This should probably go in it's own patch, or at least be mentioned in
the commit message.

Johan

Re: [PATCH V1 2/4] usb: serial: f81534: add auto RTS direction support

[Johan Hovold]

On Thu, Nov 16, 2017 at 03:46:07PM +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>
> ---
>  drivers/usb/serial/f81534.c | 54 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index 76c676ef5f0d..b2d10309c335 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -102,11 +102,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
>  
> @@ -119,6 +124,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_CLK_1_846_MHZ		BIT(0)
> @@ -126,6 +133,9 @@
>  #define F81534_CLK_24_MHZ		(BIT(0) | BIT(2))
>  #define F81534_CLK_14_77_MHZ		(BIT(0) | 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) },
> @@ -485,16 +495,20 @@ 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);
> +	struct f81534_serial_private *serial_priv;
>  	u32 divisor;
>  	int status;
>  	int idx;
>  	u8 value;
> +	u8 tmp;
>  	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};
>  
> +	serial_priv = usb_get_serial_data(port->serial);
> +
>  	do {
>  		for (idx = 0; idx < ARRAY_SIZE(baudrate_table); ++idx) {
>  			if (baudrate <= baudrate_table[idx] &&
> @@ -520,8 +534,25 @@ static int f81534_set_port_config(struct usb_serial_port *port,
>  	} while (1);
>  
>  	port_priv->baud_base = baudrate_table[idx];
> -	status = f81534_set_port_register(port, F81534_CLOCK_REG,
> -			clock_table[idx]);
> +	tmp = serial_priv->conf_data[port_priv->phy_num];
> +
> +	switch (tmp & F81534_UART_MODE_MASK) {
> +	case F81534_PORT_CONF_RS485_INVERT:
> +		value = F81534_CLK_RS485_MODE | F81534_CLK_RS485_INVERT;
> +		break;
> +	case F81534_PORT_CONF_RS485:
> +		value = F81534_CLK_RS485_MODE;
> +		break;
> +
> +	default:
> +		/* fall through, default RS232 Mode */
> +	case F81534_PORT_CONF_RS232:
> +		value = 0;
> +		break;
> +	}
> +
> +	value |= clock_table[idx];

With the shadow clock register I mentioned earlier, this can be
determined once at probe instead of at every termios change.

> +	status = f81534_set_port_register(port, F81534_CLOCK_REG, value);
>  	if (status) {
>  		dev_err(&port->dev, "CLOCK_REG setting failed\n");
>  		return status;
> @@ -1270,9 +1301,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;
> @@ -1304,6 +1338,22 @@ 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:
> +		dev_info(&port->dev, "RS485 invert mode.\n");
> +		break;
> +	case F81534_PORT_CONF_RS485:
> +		dev_info(&port->dev, "RS485 mode.\n");
> +		break;
> +
> +	default:
> +		/* fall through, default RS232 Mode */

No need for fallthrough comment here.

> +	case F81534_PORT_CONF_RS232:
> +		dev_info(&port->dev, "RS232 mode.\n");
> +		break;
> +	}
> +
>  	return 0;
>  }

Johan

Re: [PATCH V1 3/4] usb: serial: f81534: add output pin control

[Johan Hovold]

On Thu, Nov 16, 2017 at 03:46:08PM +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.
> 
> Register mapping for output value:
> 	Port 0:
> 		M2: 0x2ae8 bit7, M1: 0x2a90 bit5, M0/SD: 0x2a90 bit4
> 	Port 1:
> 		M2: 0x2ae8 bit6, M1: 0x2ae8 bit0, M0/SD: 0x2ae8 bit3
> 	Port 2:
> 		M2: 0x2a90 bit0, M1: 0x2ae8 bit2, M0/SD: 0x2a80 bit6
> 	Port 3:
> 		M2: 0x2a90 bit3, M1: 0x2a90 bit2, M0/SD: 0x2a90 bit1
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
>  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 b2d10309c335..30b966d71ae8 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -56,6 +56,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
> @@ -166,6 +167,23 @@ struct f81534_port_private {
>  	u8 phy_num;
>  };
>  
> +struct f81534_pin_data {
> +	const u16 reg_addr;
> +	const u16 reg_mask;

Should the mask not 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) } } },

Please use a space after { and before } consistently.

> +};
> +
>  static int f81534_logic_to_phy_port(struct usb_serial *serial,
>  					struct usb_serial_port *port)
>  {
> @@ -271,6 +289,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)
>  {
> @@ -1299,6 +1333,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;
> +	}

You're using 24 (get or set) accesses to update these three registers
here. Why not read them out (if necessary), determine their new values
and then write them back when done instead?

> +
> +	dev_info(&port->dev, "Output pin (M0/M1/M2): %d\n", value);

Use dev_dbg here.

Johan

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

[Johan Hovold]

On Thu, Nov 16, 2017 at 03:46:09PM +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>
> ---
>  drivers/usb/serial/f81534.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index 30b966d71ae8..18bd2a478199 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -751,6 +751,74 @@ 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 int f81534_check_port_hw_disabled(struct usb_serial *serial, int phy)

You treat errors as "disabled" so return a bool instead.

> +{
> +	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_register(serial,
> +				F81534_MODEM_STATUS_REG + phy * 0x10, &msr);

You already have a define for the magic 0x10 that you should be using.

And you also have port register accessors that take care of the offset
for you. Perhaps add another helper which takes a phy num and use that
to implement the current accessor functions?

> +	if (status)
> +		return status;
> +
> +	if ((msr & msr_mask) != msr_mask)
> +		return 0;
> +
> +	status = f81534_set_register(serial,
> +				F81534_FIFO_CONTROL_REG + phy * 0x10,
> +				UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR |
> +				UART_FCR_CLEAR_XMIT);
> +	if (status)
> +		return status;
> +
> +	status = f81534_get_register(serial,
> +				F81534_MODEM_CONTROL_REG + phy * 0x10,
> +				&old_mcr);
> +	if (status)
> +		return status;
> +
> +	status = f81534_set_register(serial,
> +				F81534_MODEM_CONTROL_REG + phy * 0x10,
> +				UART_MCR_LOOP);
> +	if (status)
> +		return status;
> +
> +	status = f81534_set_register(serial,
> +				F81534_MODEM_CONTROL_REG + phy * 0x10, 0x0);
> +	if (status)
> +		return status;
> +
> +	msleep(60);
> +
> +	status = f81534_get_register(serial,
> +				F81534_LINE_STATUS_REG + phy * 0x10, &lsr);
> +	if (status)
> +		return status;
> +
> +	status = f81534_set_register(serial,
> +				F81534_MODEM_CONTROL_REG + phy * 0x10,
> +				old_mcr);
> +	if (status)
> +		return status;
> +
> +	if ((lsr & UART_LSR_BI) == UART_LSR_BI)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +/*
>   * 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
> @@ -832,6 +900,9 @@ 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 (f81534_check_port_hw_disabled(serial, i))
> +			continue;
> +
>  		if (setting[i] & F81534_PORT_UNAVAILABLE)
>  			continue;
>  
> @@ -1306,6 +1377,9 @@ static int f81534_attach(struct usb_serial *serial)
>  
>  	/* Assign phy-to-logic mapping */
>  	for (i = 0; i < F81534_NUM_PORT; ++i) {
> +		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;

So this adds at least half a second during probe just from the msleep
alone. Can this be reduced somehow?

We should at least try to half that time by only doing that loopback
test once per port. You may need to use devres to allocate memory for
the result in calc_num_ports (or probe).

Johan

Re: [PATCH V1 3/4] usb: serial: f81534: add output pin control

[Ji-Ze Hong (Peter Hong)]

Hi Johan,

Johan Hovold 於 2017/12/19 上午 12:06 寫道:
> On Thu, Nov 16, 2017 at 03:46:08PM +0800, Ji-Ze Hong (Peter Hong) wrote:
>> +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;
>> +	}
> 
> You're using 24 (get or set) accesses to update these three registers
> here. Why not read them out (if necessary), determine their new values
> and then write them back when done instead?
> 

In this code, I'm only read/write 3 registers of 0x2ae8, 0x2a90, 0x2a80,
but some register will read/write more than once. Should I change the
code from port_probe() to attach() and re-write it as:
	1: read the 3 register
	2: change them will 12 pin desire value
	3: write it back
Is it ok?

Thanks
-- 
With Best Regards,
Peter Hong

Re: [PATCH V1 3/4] usb: serial: f81534: add output pin control

[Johan Hovold]

On Thu, Dec 21, 2017 at 05:49:45PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Hi Johan,
> 
> Johan Hovold 於 2017/12/19 上午 12:06 寫道:
> > On Thu, Nov 16, 2017 at 03:46:08PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> >> +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;
> >> +	}
> > 
> > You're using 24 (get or set) accesses to update these three registers
> > here. Why not read them out (if necessary), determine their new values
> > and then write them back when done instead?
> > 
> 
> In this code, I'm only read/write 3 registers of 0x2ae8, 0x2a90, 0x2a80,
> but some register will read/write more than once. Should I change the
> code from port_probe() to attach() and re-write it as:
> 	1: read the 3 register
> 	2: change them will 12 pin desire value
> 	3: write it back
> Is it ok?

Do you expect these pins to ever be changed after probe? If not, then
perhaps it can be moved to attach(), but otherwise I guess they should
be set at port_probe(). By using shadow registers, you should be able to
reduce the number of device accesses, but perhaps it's not worth the
complexity.

Do you have a rough idea about how long these register updates take? I
was just worried that these changes will add up to really long probe
times.

Thanks,
Johan

Re: [PATCH V1 3/4] usb: serial: f81534: add output pin control

[Ji-Ze Hong (Peter Hong)]

Hi Johan,

>> In this code, I'm only read/write 3 registers of 0x2ae8, 0x2a90, 0x2a80,
>> but some register will read/write more than once. Should I change the
>> code from port_probe() to attach() and re-write it as:
>> 	1: read the 3 register
>> 	2: change them will 12 pin desire value
>> 	3: write it back
>> Is it ok?
> 
> Do you expect these pins to ever be changed after probe? If not, then
> perhaps it can be moved to attach(), but otherwise I guess they should
> be set at port_probe(). By using shadow registers, you should be able to
> reduce the number of device accesses, but perhaps it's not worth the
> complexity.
> 
> Do you have a rough idea about how long these register updates take? I
> was just worried that these changes will add up to really long probe
> times.
> 

I had measured the time of the loop in f81534_set_port_output_pin() via
getnstimeofday() with 685.410 ~ 3681.682us per port, but normally with
600~800us per port. So I prefer remain the current method of
f81534_set_port_output_pin(). Is it ok?

Thanks
-- 
With Best Regards,
Peter Hong

Re: [PATCH V1 3/4] usb: serial: f81534: add output pin control

[Johan Hovold]

On Tue, Jan 02, 2018 at 11:24:26AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Hi Johan,
> 
> >> In this code, I'm only read/write 3 registers of 0x2ae8, 0x2a90, 0x2a80,
> >> but some register will read/write more than once. Should I change the
> >> code from port_probe() to attach() and re-write it as:
> >> 	1: read the 3 register
> >> 	2: change them will 12 pin desire value
> >> 	3: write it back
> >> Is it ok?
> > 
> > Do you expect these pins to ever be changed after probe? If not, then
> > perhaps it can be moved to attach(), but otherwise I guess they should
> > be set at port_probe(). By using shadow registers, you should be able to
> > reduce the number of device accesses, but perhaps it's not worth the
> > complexity.
> > 
> > Do you have a rough idea about how long these register updates take? I
> > was just worried that these changes will add up to really long probe
> > times.
> > 
> 
> I had measured the time of the loop in f81534_set_port_output_pin() via
> getnstimeofday() with 685.410 ~ 3681.682us per port, but normally with
> 600~800us per port. So I prefer remain the current method of
> f81534_set_port_output_pin(). Is it ok?

That should be fine. Thanks for verifying.

Johan