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.
