Re: [PATCH 4/4] serial: 8250: Add Airoha SoC UART and HSUART support

[Jiri Slaby <jirislaby@kernel.org>]

On 09. 07. 26, 22:56, Christian Marangi wrote:
...
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_airoha.c
> @@ -0,0 +1,190 @@
...
> +struct airoha_8250_priv {
> +	int line;
> +};
> +
> +struct airoha_8250_data {

Do you need this struct at all? Can't you pass the type as data directly 
(using cast)?

> +	unsigned int type;
> +};
> +
> +struct airoha_8250_clk_div_info {
> +	int div;
> +	int mask;

Perhaps make them unsigned to avoid signed arithmetics?

> +};
> +
> +#define UART_BRDL_20M		0x01
> +#define UART_BRDH_20M		0x00
> +
> +#define XINDIV_CLOCK		20000000
> +#define XYD_Y			65000
> +
> +static const struct airoha_8250_clk_div_info airoha_clk_div_info[] = {
> +	{ .div = 10, .mask = BIT(2) },
> +	{ .div = 4, .mask = BIT(1) },
> +	{ .div = 2, .mask = BIT(0) },
> +};
> +
> +static const int clock_div_tab[] = { 10, 4, 2};
> +static const int clock_div_reg[] = {  4, 2, 1};

unsigned?

> +
> +/*
> + * Airoha UART baud rate calculation logic
> + *
> + * crystal_clock = 20 MHz (fixed frequency)
> + * xindiv_clock = crystal_clock / clock_div
> + * (x/y) = XYD, 32 bit register with 16 bits of x and then 16 bits of y
> + * clock_div = XINCLK_DIVCNT (default set to 10 (0x4)),
> + *           - 3 bit register [ 1, 2, 4, 8, 10, 12, 16, 20 ]
> + *
> + * baud_rate = ((xindiv_clock) * (x/y)) / ([BRDH,BRDL] * 16)
> + *
> + * Selecting divider needs to fulfill
> + * 1.8432 MHz <= xindiv_clk <= APB clock / 2
> + * The clocks are unknown but a divider of value 1 did not result in a valid
> + * waveform.
> + *
> + * XYD_y seems to need to be larger then XYD_x for proper waveform generation.
> + * Setting [BRDH,BRDL] to [0,1] and XYD_y to 65000 gives even values
> + * for usual baud rates.
> + */
> +static void airoha_set_termios(struct uart_port *port, struct ktermios *termios,
> +			       const struct ktermios *old)
> +{
> +	const struct airoha_8250_clk_div_info *clk_div_info;
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	unsigned int xyd_x, nom, denom;
> +	unsigned int baud;
> +	int i;
> +
> +	serial8250_do_set_termios(port, termios, old);
> +
> +	baud = serial8250_get_baud_rate(port, termios, old);
> +
> +	/* Set DLAB to access the baud rate divider registers (BRDH, BRDL) */
> +	serial_port_out(port, UART_LCR, up->lcr | UART_LCR_DLAB);
> +
> +	/* Set baud rate calculation defaults (BRDIV ([BRDH,BRDL]) to 1) */
> +	serial_port_out(port, UART_AIROHA_BRDL, UART_BRDL_20M);
> +	serial_port_out(port, UART_AIROHA_BRDH, UART_BRDH_20M);
> +
> +	/*
> +	 * Calculate XYD_x and XINCLKDR register by searching
> +	 * through a table of crystal_clock divisors.
> +	 */
> +	for (i = 0 ; i < ARRAY_SIZE(airoha_clk_div_info) ; i++) {
> +		clk_div_info = &airoha_clk_div_info[i];
> +
> +		denom = (XINDIV_CLOCK / 40) / clk_div_info->div;
> +		nom = baud * (XYD_Y / 40);

Are these "/ 40" to avoid overflow? Add a comment.

> +		xyd_x = ((nom / denom) << 4);

* don't you want to round to closest instead of down?
* I don't understand the purpose of the shift though.

> +		/* For the HSUART xyd_x needs to be scaled by a factor of 2 */
> +		if (port->type == UART_PORT_AIROHA_HS)
> +			xyd_x = xyd_x >> 1;

Do not use shifts for div/mul.

> +		if (xyd_x < XYD_Y)
> +			break;
> +	}
> +
> +	serial_port_out(port, UART_AIROHA_XINCLKDR, clk_div_info->mask);
> +	serial_port_out(port, UART_AIROHA_XYD, (xyd_x << 16) | XYD_Y);
> +
> +	/* unset DLAB */
> +	serial_port_out(port, UART_LCR, up->lcr);
> +}

thanks,
-- 
js
suse labs