Date: Tue, 9 Jan 2018 12:08:41 +0100
From: Johan Hovold <johan@kernel.org>
To: "Ji-Ze Hong (Peter Hong)" <hpeter@gmail.com>
Cc: johan@kernel.org, gregkh@linuxfoundation.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        peter_hong@fintek.com.tw,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@gmail.com>
Subject: Re: [PATCH V2 1/5] usb: serial: f81534: add high baud rate support
Message-ID: <20180109110841.GN11344@localhost>
References: <1515032961-29131-1-git-send-email-hpeter+linux_kernel@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1515032961-29131-1-git-send-email-hpeter+linux_kernel@gmail.com>
User-Agent: Mutt/1.7.2 (2016-11-26)
Sender: linux-kernel-owner@vger.kernel.org

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