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

2017-12-18 Thread 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) 
> ---
>  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_START0x3000
> @@ -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_PORTBIT(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,
> + 150};
> + 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(>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

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

2017-12-18 Thread 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) 
> ---
>  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_START0x3000
> @@ -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_PORTBIT(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,
> + 150};
> + 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(>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

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

2017-11-15 Thread 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) 
---
 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_REGISTER0xA0
 
 #define F81534_NUM_PORT4
@@ -100,7 +101,6 @@
 #define F81534_CMD_READ0x03
 
 #define F81534_DEFAULT_BAUD_RATE   9600
-#define F81534_MAX_BAUDRATE115200
 
 #define F81534_PORT_CONF_DISABLE_PORT  BIT(3)
 #define F81534_PORT_CONF_NOT_EXIST_PORTBIT(7)
@@ -110,6 +110,22 @@
 #define F81534_1X_RXTRIGGER0xc3
 #define F81534_8X_RXTRIGGER0xcf
 
+/*
+ * 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,
+   150};
+   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(>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(>dev, "CLOCK_REG setting failed\n");
+   return status;
+   }
 
if (baudrate <= 1200)
value = F81534_1X_RXTRIGGER;/* 128 FIFO & TL: 1x */
@@ -486,7 +541,7 @@

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

2017-11-15 Thread 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) 
---
 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_REGISTER0xA0
 
 #define F81534_NUM_PORT4
@@ -100,7 +101,6 @@
 #define F81534_CMD_READ0x03
 
 #define F81534_DEFAULT_BAUD_RATE   9600
-#define F81534_MAX_BAUDRATE115200
 
 #define F81534_PORT_CONF_DISABLE_PORT  BIT(3)
 #define F81534_PORT_CONF_NOT_EXIST_PORTBIT(7)
@@ -110,6 +110,22 @@
 #define F81534_1X_RXTRIGGER0xc3
 #define F81534_8X_RXTRIGGER0xcf
 
+/*
+ * 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,
+   150};
+   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(>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(>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