Re: [PATCH 2/7] serial: bflb_uart: add Bouffalolab UART Driver

[Ilpo Järvinen]

On Sun, 20 Nov 2022, Jisheng Zhang wrote:

> Add the driver for Bouffalolab UART IP which is found in Bouffalolab
> SoCs such as bl808.
> 
> UART driver probe will create path named "/dev/ttySx".
> 
> Signed-off-by: Jisheng Zhang <jszhang@xxxxxxxxxx>

> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> index 238a9557b487..8509cdc11d87 100644
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_SERIAL_8250) += 8250/
>  
>  obj-$(CONFIG_SERIAL_AMBA_PL010) += amba-pl010.o
>  obj-$(CONFIG_SERIAL_AMBA_PL011) += amba-pl011.o
> +obj-$(CONFIG_SERIAL_BFLB) += bflb_uart.o
>  obj-$(CONFIG_SERIAL_CLPS711X) += clps711x.o
>  obj-$(CONFIG_SERIAL_PXA_NON8250) += pxa.o
>  obj-$(CONFIG_SERIAL_SA1100) += sa1100.o
> diff --git a/drivers/tty/serial/bflb_uart.c b/drivers/tty/serial/bflb_uart.c
> new file mode 100644
> index 000000000000..65f98ccf8fa8
> --- /dev/null
> +++ b/drivers/tty/serial/bflb_uart.c
> @@ -0,0 +1,659 @@
> +#define UART_FIFO_CONFIG_1		(0x84)
> +#define  UART_TX_FIFO_CNT_SFT		0
> +#define  UART_TX_FIFO_CNT_MSK		GENMASK(5, 0)
> +#define  UART_RX_FIFO_CNT_MSK		GENMASK(13, 8)
> +#define  UART_TX_FIFO_TH_SFT		16

Use FIELD_PREP() instead of adding a separate *_SFT define.

> +#define  UART_TX_FIFO_TH_MSK		GENMASK(20, 16)
> +#define  UART_RX_FIFO_TH_SFT		24
> +#define  UART_RX_FIFO_TH_MSK		GENMASK(28, 24)
> +#define UART_FIFO_WDATA			0x88
> +#define UART_FIFO_RDATA			0x8c
> +#define  UART_FIFO_RDATA_MSK		GENMASK(7, 0)


> +	val = rdl(port, UART_URX_CONFIG);
> +	val &= ~UART_CR_URX_EN;
> +	wrl(port, UART_URX_CONFIG, val);
> +
> +	val = rdl(port, UART_INT_MASK);
> +	val |= UART_URX_FIFO_INT | UART_URX_RTO_INT |
> +	       UART_URX_FER_INT;

Fits to single line.

> +	port->type = PORT_BFLB;
> +
> +	/* Clear mask, so no surprise interrupts. */
> +	val = rdl(port, UART_INT_MASK);
> +	val |= UART_UTX_END_INT;
> +	val |= UART_UTX_FIFO_INT;
> +	val |= UART_URX_FIFO_INT;
> +	val |= UART_URX_RTO_INT;
> +	val |= UART_URX_FER_INT;

Why to split it to this many lines?

> +	spin_lock_irqsave(&port->lock, flags);
> +
> +	val = rdl(port, UART_INT_MASK);
> +	val |= 0xfff;

In most of the other places, the bits used with UART_INT_MASK are named, 
but for some reason you don't do it here and the bits extend beyond the 
ones which are defined with name.

> +	wrl(port, UART_INT_MASK, val);
> +
> +	wrl(port, UART_DATA_CONFIG, 0);
> +	wrl(port, UART_SW_MODE, 0);
> +	wrl(port, UART_URX_RTO_TIMER, 0x4f);

FIELD_PREP(UART_CR_URX_RTO_VALUE_MSK, 0x4f)? It would document what field
is written explicitly.

> +
> +	val = rdl(port, UART_FIFO_CONFIG_1);
> +	val &= ~UART_RX_FIFO_TH_MSK;
> +	val |= BFLB_UART_RX_FIFO_TH << UART_RX_FIFO_TH_SFT;
> +	wrl(port, UART_FIFO_CONFIG_1, val);
> +
> +	/* Unmask RX interrupts now */
> +	val = rdl(port, UART_INT_MASK);
> +	val &= ~UART_URX_FIFO_INT;
> +	val &= ~UART_URX_RTO_INT;
> +	val &= ~UART_URX_FER_INT;

Combine to single line.

> +static int bflb_uart_request_port(struct uart_port *port)
> +{
> +	/* UARTs always present */
> +	return 0;
> +}
> +static void bflb_uart_release_port(struct uart_port *port)
> +{
> +	/* Nothing to release... */
> +}

Both release_port and request_port are NULL checked by the caller, there's 
no need to provide and empty one.

> +static const struct uart_ops bflb_uart_ops = {
> +	.tx_empty = bflb_uart_tx_empty,
> +	.get_mctrl = bflb_uart_get_mctrl,
> +	.set_mctrl = bflb_uart_set_mctrl,
> +	.start_tx = bflb_uart_start_tx,
> +	.stop_tx = bflb_uart_stop_tx,
> +	.stop_rx = bflb_uart_stop_rx,
> +	.break_ctl = bflb_uart_break_ctl,
> +	.startup = bflb_uart_startup,
> +	.shutdown = bflb_uart_shutdown,
> +	.set_termios = bflb_uart_set_termios,
> +	.type = bflb_uart_type,
> +	.request_port = bflb_uart_request_port,
> +	.release_port = bflb_uart_release_port,
> +	.config_port = bflb_uart_config_port,
> +	.verify_port = bflb_uart_verify_port,
> +};

> +static void bflb_uart_console_write(struct console *co, const char *s,
> +				    u_int count)
> +{
> +	struct uart_port *port = &bflb_uart_ports[co->index]->port;
> +	u32 status, reg, mask;
> +
> +	/* save then disable interrupts */
> +	mask = rdl(port, UART_INT_MASK);
> +	reg = -1;

Use ~0 instead. Why -1 here and 0xfff in the other place?


-- 
 i.