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.