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

From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Jisheng Zhang <jszhang@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	 Rob Herring <robh+dt@kernel.org>,
	 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	 Paul Walmsley <paul.walmsley@sifive.com>,
	 Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	 Jiri Slaby <jirislaby@kernel.org>,
	 linux-serial <linux-serial@vger.kernel.org>,
	devicetree@vger.kernel.org,  LKML <linux-kernel@vger.kernel.org>,
	linux-riscv@lists.infradead.org
Subject: Re: [PATCH 2/7] serial: bflb_uart: add Bouffalolab UART Driver
Date: Mon, 21 Nov 2022 15:59:13 +0200 (EET)	[thread overview]
Message-ID: <faa34e87-4633-31e7-144b-4fec46cb8f59@linux.intel.com> (raw)
In-Reply-To: <20221120082114.3030-3-jszhang@kernel.org>

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@kernel.org>

> 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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv