Hi, I'll not note all things below because others have already seemingly commented many things. On Wed, 15 Mar 2023, Jacky Huang wrote: > From: Jacky Huang > > This adds UART and console driver for Nuvoton ma35d1 Soc. > > MA35D1 SoC provides up to 17 UART controllers, each with one uart port. > The ma35d1 uart controller is not compatible with 8250. > The uart controller supports: > - Full-duplex asynchronous communications > - Separates tx and tx 32/32 bytes entry FIFO for data payloads > - Hardware auto-flow control > - Programmable rx buffer trigger level (1/4/8/14/30 bytes) > - Individual programmable baud rate generator for each channel > - Supports nCTS, incoming data, rx FIFO reached threshold and > RS-485 Address Match (AAD mode) wake-up function > - Supports 8-bit rx buffer time-out detection function > - Programmable tx data delay time > - Supports Auto-Baud Rate measurement and baud rate compensation > - Supports break error, frame error, parity error and rx/tx buffer > overflow detection function > – Programmable number of data bit, 5-, 6-, 7-, 8- bit character > – Programmable parity bit, even, odd, no parity or stick parity bit > generation and detection > – Programmable stop bit, 1, 1.5, or 2 stop bit generation > - Supports IrDA SIR function mode > - Supports RS-485 function mode > – Supports RS-485 9-bit mode > – Supports hardware or software enables to program nRTS pin to control > RS-485 transmission direction > - Supports PDMA transfer function > - Support Single-wire function mode. This list is probably copy-pasted from somewhere but it doesn't match what you implemented in the driver. > Signed-off-by: Jacky Huang > --- > drivers/tty/serial/Kconfig | 18 + > drivers/tty/serial/Makefile | 1 + > drivers/tty/serial/ma35d1_serial.c | 842 +++++++++++++++++++++++++++++ > drivers/tty/serial/ma35d1_serial.h | 93 ++++ > include/uapi/linux/serial_core.h | 3 + > 5 files changed, 957 insertions(+) > create mode 100644 drivers/tty/serial/ma35d1_serial.c > create mode 100644 drivers/tty/serial/ma35d1_serial.h > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > index 625358f44419..cb47fe804595 100644 > --- a/drivers/tty/serial/Kconfig > +++ b/drivers/tty/serial/Kconfig > @@ -1562,6 +1562,24 @@ config SERIAL_SUNPLUS_CONSOLE > you can alter that using a kernel command line option such as > "console=ttySUPx". > > +config SERIAL_NUVOTON_MA35D1 > + tristate "Nuvoton MA35D1 family UART support" > + depends on ARCH_NUVOTON || COMPILE_TEST > + select SERIAL_CORE > + help > + This driver supports Nuvoton MA35D1 family UART ports. If you would > + like to use them, you must answer Y or M to this option. Note that > + for use as console, it must be included in kernel and not as a > + module > + > +config SERIAL_NUVOTON_MA35D1_CONSOLE > + bool "Console on a Nuvotn MA35D1 family UART port" > + depends on SERIAL_NUVOTON_MA35D1=y > + select SERIAL_CORE_CONSOLE > + help > + Select this options if you'd like to use the UART port0 of the > + Nuvoton MA35D1 family as a console. > + > endmenu > > config SERIAL_MCTRL_GPIO > diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile > index cd9afd9e3018..71ebeba06ff2 100644 > --- a/drivers/tty/serial/Makefile > +++ b/drivers/tty/serial/Makefile > @@ -93,3 +93,4 @@ obj-$(CONFIG_SERIAL_MCTRL_GPIO) += serial_mctrl_gpio.o > > obj-$(CONFIG_SERIAL_KGDB_NMI) += kgdb_nmi.o > obj-$(CONFIG_KGDB_SERIAL_CONSOLE) += kgdboc.o > +obj-$(CONFIG_SERIAL_NUVOTON_MA35D1) += ma35d1_serial.o > diff --git a/drivers/tty/serial/ma35d1_serial.c b/drivers/tty/serial/ma35d1_serial.c > new file mode 100644 > index 000000000000..8940d07c3777 > --- /dev/null > +++ b/drivers/tty/serial/ma35d1_serial.c > @@ -0,0 +1,842 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * MA35D1 serial driver > + * Copyright (C) 2023 Nuvoton Technology Corp. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include "ma35d1_serial.h" > + > +#define UART_NR 17 > + > +static struct uart_driver ma35d1serial_reg; > +struct clk *clk; > + > +struct uart_ma35d1_port { > + struct uart_port port; > + u16 capabilities; /* port capabilities */ > + u8 ier; > + u8 lcr; > + u8 mcr; > + u8 mcr_mask; /* mask of user bits */ > + u8 mcr_force; /* mask of forced bits */ > + struct serial_rs485 rs485; /* rs485 settings */ > + u32 baud_rate; > + int rx_count; > + u32 console_baud_rate; > + u32 console_line; > + u32 console_int; > +}; > + > +static struct device_node *ma35d1serial_uart_nodes[UART_NR]; > +static struct uart_ma35d1_port ma35d1serial_ports[UART_NR] = { 0 }; > +static void __stop_tx(struct uart_ma35d1_port *p); > +static void transmit_chars(struct uart_ma35d1_port *up); Try to rearrange such that forward declarations are not necessary for functions. > +static struct uart_ma35d1_port *to_ma35d1_uart_port(struct uart_port *uart) static inline > +{ > + return container_of(uart, struct uart_ma35d1_port, port); > +} > + > +static u32 serial_in(struct uart_ma35d1_port *p, int offset) > +{ > + return __raw_readl(p->port.membase + offset); > +} > + > +static void serial_out(struct uart_ma35d1_port *p, int offset, int value) > +{ > + __raw_writel(value, p->port.membase + offset); > +} > + > +static void __stop_tx(struct uart_ma35d1_port *p) > +{ > + u32 ier; > + > + ier = serial_in(p, UART_REG_IER); > + if (ier & THRE_IEN) > + serial_out(p, UART_REG_IER, ier & ~THRE_IEN); > +} > + > +static void ma35d1serial_stop_tx(struct uart_port *port) > +{ > + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; > + > + __stop_tx(up); > +} > + > +static void ma35d1serial_start_tx(struct uart_port *port) > +{ > + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; > + u32 ier; > + struct circ_buf *xmit = &up->port.state->xmit; > + > + ier = serial_in(up, UART_REG_IER); > + serial_out(up, UART_REG_IER, ier & ~THRE_IEN); > + if (uart_circ_chars_pending(xmit) < > + (16 - ((serial_in(up, UART_REG_FSR) >> 16) & 0x3F))) > + transmit_chars(up); > + serial_out(up, UART_REG_IER, ier | THRE_IEN); > +} > + > +static void ma35d1serial_stop_rx(struct uart_port *port) > +{ > + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; > + > + serial_out(up, UART_REG_IER, serial_in(up, UART_REG_IER) & ~RDA_IEN); > +} > + > +static void > +receive_chars(struct uart_ma35d1_port *up) > +{ > + u8 ch; > + u32 fsr; > + u32 isr; > + u32 dcnt; > + char flag; > + > + isr = serial_in(up, UART_REG_ISR); > + fsr = serial_in(up, UART_REG_FSR); > + > + while (!(fsr & RX_EMPTY)) { > + flag = TTY_NORMAL; > + up->port.icount.rx++; > + > + if (unlikely(fsr & (BIF | FEF | PEF | RX_OVER_IF))) { > + if (fsr & BIF) { > + serial_out(up, UART_REG_FSR, BIF); > + up->port.icount.brk++; > + if (uart_handle_break(&up->port)) > + continue; > + } > + if (fsr & FEF) { > + serial_out(up, UART_REG_FSR, FEF); > + up->port.icount.frame++; > + } > + if (fsr & PEF) { > + serial_out(up, UART_REG_FSR, PEF); > + up->port.icount.parity++; > + } > + if (fsr & RX_OVER_IF) { > + serial_out(up, UART_REG_FSR, RX_OVER_IF); > + up->port.icount.overrun++; > + } Do you need to write each of those individually to clear(?) them or could you just do one write here after the accounting is done: serial_out(up, UART_REG_FSR, fsr & (BIF|FEF|PEF|RX_OVER_IF)); ? Also, add some driver specific prefix for the flag naming (all driver specific ones, not just these if you have others besides these). > + if (fsr & BIF) > + flag = TTY_BREAK; > + if (fsr & PEF) > + flag = TTY_PARITY; > + if (fsr & FEF) > + flag = TTY_FRAME; Are you sure this is the right prioritization or do you perhaps want else ifs like in some other serial drivers? > + } > + ch = (u8)serial_in(up, UART_REG_RBR); Drop the case. > + if (uart_handle_sysrq_char(&up->port, ch)) > + continue; > + > + uart_insert_char(&up->port, fsr, RX_OVER_IF, ch, flag); > + up->rx_count++; > + dcnt = (serial_in(up, UART_REG_FSR) >> 8) & 0x3f; > + if (up->rx_count > 1023) { > + spin_lock(&up->port.lock); > + tty_flip_buffer_push(&up->port.state->port); > + spin_unlock(&up->port.lock); > + up->rx_count = 0; Why is all this ->rx_count trickery necessary? What's so special with the size in question? > + if ((isr & RXTO_IF) && (dcnt == 0)) > + goto tout_end; > + } > + if (isr & RDA_IF) { > + if (dcnt == 1) Merge to the same comdition. dcnt could probably have a more descriptive name. > + return; > + } > + fsr = serial_in(up, UART_REG_FSR); > + } > + spin_lock(&up->port.lock); > + tty_flip_buffer_push(&up->port.state->port); > + spin_unlock(&up->port.lock); > +tout_end: > + up->rx_count = 0; > +} > + > +static void transmit_chars(struct uart_ma35d1_port *up) > +{ > + struct circ_buf *xmit = &up->port.state->xmit; > + int count = 16 - ((serial_in(up, UART_REG_FSR) >> 16) & 0xF); > + > + if (serial_in(up, UART_REG_FSR) & TX_FULL) > + count = 0; > + if (up->port.x_char) { > + serial_out(up, UART_REG_THR, up->port.x_char); > + up->port.icount.tx++; > + up->port.x_char = 0; > + return; > + } > + if (uart_tx_stopped(&up->port)) { > + ma35d1serial_stop_tx(&up->port); > + return; > + } > + if (uart_circ_empty(xmit)) { > + __stop_tx(up); > + return; > + } > + while (count > 0) { > + serial_out(up, UART_REG_THR, xmit->buf[xmit->tail]); > + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); > + up->port.icount.tx++; > + count--; > + if (uart_circ_empty(xmit)) > + break; > + } > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > + uart_write_wakeup(&up->port); > + if (uart_circ_empty(xmit)) > + __stop_tx(up); > +} > + > +static irqreturn_t ma35d1serial_interrupt(int irq, void *dev_id) > +{ > + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)dev_id; > + u32 isr, fsr; > + > + isr = serial_in(up, UART_REG_ISR); > + fsr = serial_in(up, UART_REG_FSR); > + if (isr & (RDA_IF | RXTO_IF)) > + receive_chars(up); > + if (isr & THRE_INT) > + transmit_chars(up); > + if (fsr & (BIF | FEF | PEF | RX_OVER_IF | TX_OVER_IF)) > + serial_out(up, UART_REG_FSR, > + (BIF | FEF | PEF | RX_OVER_IF | TX_OVER_IF)); Hmm... Why write these again here... Didn't receive_chars() already clear(?) most of these bits?? > + > + return IRQ_HANDLED; > +} > + > +static u32 ma35d1serial_tx_empty(struct uart_port *port) > +{ > + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; > + u32 fsr; > + > + fsr = serial_in(up, UART_REG_FSR); > + return (fsr & (TE_FLAG | TX_EMPTY)) == (TE_FLAG | TX_EMPTY) ? > + TIOCSER_TEMT : 0; > +} > + > +static u32 ma35d1serial_get_mctrl(struct uart_port *port) > +{ > + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; > + u32 status; > + u32 ret = 0; > + > + status = serial_in(up, UART_REG_MSR); > + if (!(status & 0x10)) > + ret |= TIOCM_CTS; > + return ret; > +} > + > +static void ma35d1serial_set_mctrl(struct uart_port *port, u32 mctrl) > +{ > + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; > + u32 mcr = 0; > + u32 ier = 0; > + > + if (mctrl & TIOCM_RTS) { > + /* set RTS high level trigger */ > + mcr = serial_in(up, UART_REG_MCR); > + mcr |= 0x200; > + mcr &= ~(0x2); > + } > + if (up->mcr & UART_MCR_AFE) { > + /* set RTS high level trigger */ > + mcr = serial_in(up, UART_REG_MCR); > + mcr |= 0x200; > + mcr &= ~(0x2); > + > + /* enable CTS/RTS auto-flow control */ > + serial_out(up, UART_REG_IER, > + (serial_in(up, UART_REG_IER) | (0x3000))); Once you have named the bits probably with defines, most of the comments such as the one above are rendered redundant and should be dropped. > + > + /* Set hardware flow control */ > + up->port.flags |= UPF_HARD_FLOW; > + } else { > + /* disable CTS/RTS auto-flow control */ > + ier = serial_in(up, UART_REG_IER); > + ier &= ~(0x3000); > + serial_out(up, UART_REG_IER, ier); > + > + /* un-set hardware flow control */ > + up->port.flags &= ~UPF_HARD_FLOW; > + } > + > + /* set CTS high level trigger */ > + serial_out(up, UART_REG_MSR, (serial_in(up, UART_REG_MSR) | (0x100))); > + serial_out(up, UART_REG_MCR, mcr); > +} > + > +static void ma35d1serial_break_ctl(struct uart_port *port, int break_state) > +{ > + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; > + unsigned long flags; > + u32 lcr; > + > + spin_lock_irqsave(&up->port.lock, flags); > + lcr = serial_in(up, UART_REG_LCR); > + if (break_state != 0) > + lcr |= BCB; /* set break */ > + else > + lcr &= ~BCB; /* clr break */ While BCB might come from HW naming, a better name for would make these very obvious (and the comments unnecessary). > + serial_out(up, UART_REG_LCR, lcr); > + spin_unlock_irqrestore(&up->port.lock, flags); > +} > + > +static int ma35d1serial_startup(struct uart_port *port) > +{ > + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; > + struct tty_struct *tty = port->state->port.tty; > + int retval; > + > + /* Reset FIFO */ > + serial_out(up, UART_REG_FCR, TFR | RFR /* | RX_DIS */); > + > + /* Clear pending interrupts */ > + serial_out(up, UART_REG_ISR, 0xFFFFFFFF); ~0 is another option. > + > + retval = request_irq(port->irq, ma35d1serial_interrupt, 0, > + tty ? tty->name : "ma35d1_serial", port); Why such exceptional name trickery? > + if (retval) { > + dev_err(up->port.dev, "request irq failed.\n"); > + return retval; > + } > + > + /* Now, initialize the UART */ > + /* FIFO trigger level 4 byte */ > + /* RTS trigger level 8 bytes */ > + serial_out(up, UART_REG_FCR, > + serial_in(up, UART_REG_FCR) | 0x10 | 0x20000); > + serial_out(up, UART_REG_LCR, 0x7); /* 8 bit */ > + serial_out(up, UART_REG_TOR, 0x40); > + serial_out(up, UART_REG_IER, > + RTO_IEN | RDA_IEN | TIME_OUT_EN | BUFERR_IEN); > + return 0; > +} > + > +static void ma35d1serial_shutdown(struct uart_port *port) > +{ > + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; > + > + free_irq(port->irq, port); > + > + /* Disable interrupts from this port */ > + serial_out(up, UART_REG_IER, 0); > +} > + > +static u32 ma35d1serial_get_divisor(struct uart_port *port, u32 baud) > +{ > + u32 quot; > + > + quot = (port->uartclk / baud) - 2; Unnecessary parenthesis. (+Somebody already commented about the unnecessary variable.) > + return quot; > +} > + > +static void ma35d1serial_set_termios(struct uart_port *port, > + struct ktermios *termios, > + const struct ktermios *old) > +{ > + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; > + u32 lcr = 0; > + unsigned long flags; > + u32 baud, quot; > + > + switch (termios->c_cflag & CSIZE) { > + case CS5: > + lcr = 0; > + break; > + case CS6: > + lcr |= 1; > + break; > + case CS7: > + lcr |= 2; > + break; > + case CS8: > + default: > + lcr |= 3; > + break; > + } > + > + if (termios->c_cflag & CSTOPB) > + lcr |= NSB; > + if (termios->c_cflag & PARENB) > + lcr |= PBE; > + if (!(termios->c_cflag & PARODD)) > + lcr |= EPE; > + if (termios->c_cflag & CMSPAR) > + lcr |= SPE; > + > + baud = uart_get_baud_rate(port, termios, old, port->uartclk / 0xffff, > + port->uartclk / 11); > + > + quot = ma35d1serial_get_divisor(port, baud); > + > + /* > + * Ok, we're now changing the port state. Do it with > + * interrupts disabled. > + */ > + spin_lock_irqsave(&up->port.lock, flags); > + > + up->port.read_status_mask = RX_OVER_IF; > + if (termios->c_iflag & INPCK) > + up->port.read_status_mask |= FEF | PEF; > + if (termios->c_iflag & (BRKINT | PARMRK)) > + up->port.read_status_mask |= BIF; > + > + /* > + * Characteres to ignore > + */ > + up->port.ignore_status_mask = 0; > + if (termios->c_iflag & IGNPAR) > + up->port.ignore_status_mask |= FEF | PEF; > + if (termios->c_iflag & IGNBRK) { > + up->port.ignore_status_mask |= BIF; > + /* > + * If we're ignoring parity and break indicators, > + * ignore overruns too (for real raw support). > + */ > + if (termios->c_iflag & IGNPAR) > + up->port.ignore_status_mask |= RX_OVER_IF; > + } > + if (termios->c_cflag & CRTSCTS) > + up->mcr |= UART_MCR_AFE; > + else > + up->mcr &= ~UART_MCR_AFE; > + > + ma35d1serial_set_mctrl(&up->port, up->port.mctrl); > + serial_out(up, UART_REG_BAUD, quot | 0x30000000); > + serial_out(up, UART_REG_LCR, lcr); You need to do uart_update_timeout() in the set_termios function. > + spin_unlock_irqrestore(&up->port.lock, flags); > +} > + > +static void ma35d1serial_release_port(struct uart_port *port) > +{ > + iounmap(port->membase); > + port->membase = NULL; > +} > + > +static int ma35d1serial_request_port(struct uart_port *port) > +{ > + return 0; > +} > + > +static void ma35d1serial_config_port(struct uart_port *port, int flags) > +{ > + int ret; > + > + /* > + * Find the region that we can probe for. This in turn > + * tells us whether we can probe for the type of port. > + */ > + ret = ma35d1serial_request_port(port); > + if (ret < 0) > + return; > + port->type = PORT_MA35D1; > +} > + > +static int ma35d1serial_verify_port(struct uart_port *port, > + struct serial_struct *ser) > +{ > + if (ser->type != PORT_UNKNOWN && ser->type != PORT_MA35D1) > + return -EINVAL; > + return 0; > +} > + > +static const char *ma35d1serial_type(struct uart_port *port) > +{ > + return (port->type == PORT_MA35D1) ? "MA35D1" : NULL; > +} > + > +/* Enable or disable the rs485 support */ > +static int ma35d1serial_config_rs485(struct uart_port *port, > + struct ktermios *termios, > + struct serial_rs485 *rs485conf) > +{ > + struct uart_ma35d1_port *p = to_ma35d1_uart_port(port); > + > + p->rs485 = *rs485conf; > + > + if (p->rs485.delay_rts_before_send >= 1000) > + p->rs485.delay_rts_before_send = 1000; Don't do this in driver, the core handles the delay limits. You don't seem to be using the value anyway for anything??? Please separate the RS485 support into its own patch. > + serial_out(p, UART_FUN_SEL, > + (serial_in(p, UART_FUN_SEL) & ~FUN_SEL_MASK)); > + > + if (rs485conf->flags & SER_RS485_ENABLED) { > + serial_out(p, UART_FUN_SEL, > + (serial_in(p, UART_FUN_SEL) | FUN_SEL_RS485)); Does this pair of serial_out()s glitch the RS485 line if ->rs485_config() is called while RS485 mode is already set? Why you need to do serial_in() from the UART_FUN_SEL twice? > + > + if (rs485conf->flags & SER_RS485_RTS_ON_SEND) > + serial_out(p, UART_REG_MCR, > + (serial_in(p, UART_REG_MCR) & ~0x200)); > + else > + serial_out(p, UART_REG_MCR, > + (serial_in(p, UART_REG_MCR) | 0x200)); > + > + /* set auto direction mode */ > + serial_out(p, UART_REG_ALT_CSR, > + (serial_in(p, UART_REG_ALT_CSR) | (1 << 10))); > + } > + return 0; > +} > + > +static int ma35d1serial_ioctl(struct uart_port *port, u32 cmd, unsigned long arg) > +{ > + switch (cmd) { > + default: > + return -ENOIOCTLCMD; > + } > + return 0; > +} > + > +static const struct uart_ops ma35d1serial_ops = { > + .tx_empty = ma35d1serial_tx_empty, > + .set_mctrl = ma35d1serial_set_mctrl, > + .get_mctrl = ma35d1serial_get_mctrl, > + .stop_tx = ma35d1serial_stop_tx, > + .start_tx = ma35d1serial_start_tx, > + .stop_rx = ma35d1serial_stop_rx, > + .break_ctl = ma35d1serial_break_ctl, > + .startup = ma35d1serial_startup, > + .shutdown = ma35d1serial_shutdown, > + .set_termios = ma35d1serial_set_termios, > + .type = ma35d1serial_type, > + .release_port = ma35d1serial_release_port, > + .request_port = ma35d1serial_request_port, > + .config_port = ma35d1serial_config_port, > + .verify_port = ma35d1serial_verify_port, > + .ioctl = ma35d1serial_ioctl, > +}; > + > +static const struct of_device_id ma35d1_serial_of_match[] = { > + { .compatible = "nuvoton,ma35d1-uart" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, ma35d1_serial_of_match); > + > +#ifdef CONFIG_SERIAL_NUVOTON_MA35D1_CONSOLE > + > +static void ma35d1serial_console_putchar(struct uart_port *port, > + unsigned char ch) > +{ > + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; > + > + do { > + } while ((serial_in(up, UART_REG_FSR) & TX_FULL)); > + serial_out(up, UART_REG_THR, ch); > +} > + > +/* > + * Print a string to the serial port trying not to disturb > + * any possible real use of the port... > + * > + * The console_lock must be held when we get here. > + */ > +static void ma35d1serial_console_write(struct console *co, > + const char *s, u32 count) > +{ > + struct uart_ma35d1_port *up = &ma35d1serial_ports[co->index]; > + unsigned long flags; > + u32 ier; > + > + local_irq_save(flags); > + > + /* > + * First save the IER then disable the interrupts > + */ > + ier = serial_in(up, UART_REG_IER); > + serial_out(up, UART_REG_IER, 0); > + > + uart_console_write(&up->port, s, count, ma35d1serial_console_putchar); > + > + /* > + * Finally, wait for transmitter to become empty > + * and restore the IER > + */ > + do { > + } while (!(serial_in(up, UART_REG_FSR) & TX_EMPTY)); > + serial_out(up, UART_REG_IER, ier); > + local_irq_restore(flags); > +} > + > +static int __init ma35d1serial_console_setup(struct console *co, > + char *options) > +{ > + struct device_node *np = ma35d1serial_uart_nodes[co->index]; > + struct uart_ma35d1_port *p = &ma35d1serial_ports[co->index]; > + u32 val32[4]; > + struct uart_port *port; > + int baud = 115200; > + int bits = 8; > + int parity = 'n'; > + int flow = 'n'; > + > + /* > + * Check whether an invalid uart number has been specified, and > + * if so, search for the first available port that does have > + * console support. > + */ > + if ((co->index < 0) || (co->index >= UART_NR)) { > + pr_debug("Console Port%x out of range\n", co->index); > + return -EINVAL; > + } > + > + if (of_property_read_u32_array(np, "reg", val32, 4) != 0) > + return -EINVAL; > + p->port.iobase = val32[1]; > + p->port.membase = ioremap(p->port.iobase, 0x10000); > + p->port.ops = &ma35d1serial_ops; > + p->port.line = 0; > + p->port.uartclk = 24000000; > + > + port = &ma35d1serial_ports[co->index].port; > + return uart_set_options(port, co, baud, parity, bits, flow); > +} > + > +static struct console ma35d1serial_console = { > + .name = "ttyS", > + .write = ma35d1serial_console_write, > + .device = uart_console_device, > + .setup = ma35d1serial_console_setup, > + .flags = CON_PRINTBUFFER | CON_ENABLED, > + .index = -1, > + .data = &ma35d1serial_reg, > +}; > + > +static void > +ma35d1serial_console_init_port(void) > +{ > + int i = 0; > + struct device_node *np; > + > + for_each_matching_node(np, ma35d1_serial_of_match) { > + if (ma35d1serial_uart_nodes[i] == NULL) { > + ma35d1serial_uart_nodes[i] = np; > + i++; > + } > + } > +} > + > +static int __init ma35d1serial_console_init(void) > +{ > + ma35d1serial_console_init_port(); > + register_console(&ma35d1serial_console); > + return 0; > +} > +console_initcall(ma35d1serial_console_init); > + > +#define MA35D1SERIAL_CONSOLE (&ma35d1serial_console) > +#else > +#define MA35D1SERIAL_CONSOLE NULL > +#endif > + > +static struct uart_driver ma35d1serial_reg = { > + .owner = THIS_MODULE, > + .driver_name = "serial", > + .dev_name = "ttyS", > + .major = TTY_MAJOR, > + .minor = 64, > + .cons = MA35D1SERIAL_CONSOLE, > + .nr = UART_NR, > +}; > + > +/** > + * Suspend one serial port. > + */ > +void ma35d1serial_suspend_port(int line) > +{ > + uart_suspend_port(&ma35d1serial_reg, &ma35d1serial_ports[line].port); > +} > +EXPORT_SYMBOL(ma35d1serial_suspend_port); > + > +/** > + * Resume one serial port. > + */ > +void ma35d1serial_resume_port(int line) > +{ > + struct uart_ma35d1_port *up = &ma35d1serial_ports[line]; > + > + uart_resume_port(&ma35d1serial_reg, &up->port); > +} > +EXPORT_SYMBOL(ma35d1serial_resume_port); > + > +/* > + * Register a set of serial devices attached to a platform device. > + * The list is terminated with a zero flags entry, which means we expect > + * all entries to have at least UPF_BOOT_AUTOCONF set. > + */ > +static int ma35d1serial_probe(struct platform_device *pdev) > +{ > + struct resource *res_mem; > + struct uart_ma35d1_port *up; > + int ret; > + struct clk *clk; > + int err; > + > + if (pdev->dev.of_node) { > + ret = of_alias_get_id(pdev->dev.of_node, "serial"); > + if (ret < 0) { > + dev_err(&pdev->dev, > + "failed to get alias/pdev id, errno %d\n", > + ret); Just put error prints to one line if you don't break 100 chars limit. > + return ret; Misaligned line. > + } > + } > + up = &ma35d1serial_ports[ret]; > + up->port.line = ret; > + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res_mem) > + return -ENODEV; > + > + up->port.iobase = res_mem->start; > + up->port.membase = ioremap(up->port.iobase, 0x10000); Define for the literal. > + up->port.ops = &ma35d1serial_ops; > + > + spin_lock_init(&up->port.lock); > + > + clk = of_clk_get(pdev->dev.of_node, 0); > + if (IS_ERR(clk)) { > + err = PTR_ERR(clk); > + dev_err(&pdev->dev, "failed to get core clk: %d\n", err); > + return -ENOENT; > + } > + err = clk_prepare_enable(clk); > + if (err) > + return -ENOENT; > + > + if (up->port.line != 0) > + up->port.uartclk = clk_get_rate(clk); > + up->port.irq = platform_get_irq(pdev, 0); > + up->port.dev = &pdev->dev; > + up->port.flags = UPF_BOOT_AUTOCONF; > + up->port.rs485_config = ma35d1serial_config_rs485; Please provide also .rs485_supported for the serial core to handle the supported features for you. > + ret = uart_add_one_port(&ma35d1serial_reg, &up->port); > + platform_set_drvdata(pdev, up); > + return 0; > +} > + > +/* > + * Remove serial ports registered against a platform device. > + */ > +static int ma35d1serial_remove(struct platform_device *dev) > +{ > + int i; > + struct uart_port *port = platform_get_drvdata(dev); > + > + free_irq(port->irq, port); > + for (i = 0; i < UART_NR; i++) { > + struct uart_ma35d1_port *up = &ma35d1serial_ports[i]; > + > + if (up->port.dev == &dev->dev) > + uart_remove_one_port(&ma35d1serial_reg, &up->port); > + } > + return 0; > +} > + > +static int ma35d1serial_suspend(struct platform_device *dev, > + pm_message_t state) > +{ > + int i; > + struct uart_ma35d1_port *up; > + > + if (dev->dev.of_node) > + i = of_alias_get_id(dev->dev.of_node, "serial"); > + if (i < 0) { > + dev_err(&dev->dev, "failed to get alias/pdev id, errno %d\n", > + i); Just put this to the same line with the rest. > + return i; > + } > + up = &ma35d1serial_ports[i]; > + if (i == 0) { > + up->console_baud_rate = serial_in(up, UART_REG_BAUD); > + up->console_line = serial_in(up, UART_REG_LCR); > + up->console_int = serial_in(up, UART_REG_IER); > + } > + return 0; > +} > + > +static int ma35d1serial_resume(struct platform_device *dev) > +{ > + int i; > + struct uart_ma35d1_port *up; > + > + if (dev->dev.of_node) > + i = of_alias_get_id(dev->dev.of_node, "serial"); > + if (i < 0) { > + dev_err(&dev->dev, "failed to get alias/pdev id, errno %d\n", > + i); Same line. > + return i; > + } > + up = &ma35d1serial_ports[i]; > + if (i == 0) { > + serial_out(up, UART_REG_BAUD, up->console_baud_rate); > + serial_out(up, UART_REG_LCR, up->console_line); > + serial_out(up, UART_REG_IER, up->console_int); > + } > + return 0; > +} > + > +static struct platform_driver ma35d1serial_driver = { > + .probe = ma35d1serial_probe, > + .remove = ma35d1serial_remove, > + .suspend = ma35d1serial_suspend, > + .resume = ma35d1serial_resume, > + .driver = { > + .name = "ma35d1-uart", > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(ma35d1_serial_of_match), > + }, > +}; > + > +static int __init ma35d1serial_init(void) > +{ > + int ret; > + > + ret = uart_register_driver(&ma35d1serial_reg); > + if (ret) > + return ret; > + ret = platform_driver_register(&ma35d1serial_driver); > + if (ret) > + uart_unregister_driver(&ma35d1serial_reg); > + return ret; > +} > + > +static void __exit ma35d1serial_exit(void) > +{ > + platform_driver_unregister(&ma35d1serial_driver); > + uart_unregister_driver(&ma35d1serial_reg); > +} > + > +module_init(ma35d1serial_init); > +module_exit(ma35d1serial_exit); > + > +MODULE_LICENSE("GPL v2"); "GPL" is enough for MODULE_LICENSE, the SPDX at the start of file covers more specific license variations. > +MODULE_DESCRIPTION("MA35D1 serial driver"); > +MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR); > + > diff --git a/drivers/tty/serial/ma35d1_serial.h b/drivers/tty/serial/ma35d1_serial.h > new file mode 100644 > index 000000000000..5fd845c31b29 > --- /dev/null > +++ b/drivers/tty/serial/ma35d1_serial.h > @@ -0,0 +1,93 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * MA35D1 serial driver header file > + * Copyright (C) 2023 Nuvoton Technology Corp. > + */ > +#ifndef __MA35D1_SERIAL_H__ > +#define __MA35D1_SERIAL_H__ > + > +/* UART Receive/Transmit Buffer Register */ > +#define UART_REG_RBR 0x00 > +#define UART_REG_THR 0x00 > + > +/* UART Interrupt Enable Register */ > +#define UART_REG_IER 0x04 > +#define RDA_IEN 0x00000001 /* RBR Available Interrupt Enable */ > +#define THRE_IEN 0x00000002 /* THR Empty Interrupt Enable */ > +#define RLS_IEN 0x00000004 /* RX Line Status Interrupt Enable */ > +#define RTO_IEN 0x00000010 /* RX Time-out Interrupt Enable */ > +#define BUFERR_IEN 0x00000020 /* Buffer Error Interrupt Enable */ > +#define TIME_OUT_EN 0x00000800 /* RX Buffer Time-out Counter Enable */ > + > +/* UART FIFO Control Register */ > +#define UART_REG_FCR 0x08 > +#define RFR 0x00000002 /* RX Field Software Reset */ > +#define TFR 0x00000004 /* TX Field Software Reset */ > + > +/* UART Line Control Register */ > +#define UART_REG_LCR 0x0C > +#define NSB 0x00000004 /* Number of “STOP Bit” */ > +#define PBE 0x00000008 /* Parity Bit Enable */ > +#define EPE 0x00000010 /* Even Parity Enable */ > +#define SPE 0x00000020 /* Stick Parity Enable */ > +#define BCB 0x00000040 /* Break Control */ > + > +/* UART Modem Control Register */ > +#define UART_REG_MCR 0x10 > +#define RTS 0x00000020 /* nRTS Signal Control */ > +#define RTSACTLV 0x00000200 /* nRTS Pin Active Level */ > +#define RTSSTS 0x00002000 /* nRTS Pin Status (Read Only) */ > + > +/* UART Modem Status Register */ > +#define UART_REG_MSR 0x14 > +#define CTSDETF 0x00000001 /* Detect nCTS State Change Flag */ > +#define CTSSTS 0x00000010 /* nCTS Pin Status (Read Only) */ > +#define CTSACTLV 0x00000100 /* nCTS Pin Active Level */ > + > +/* UART FIFO Status Register */ > +#define UART_REG_FSR 0x18 > +#define RX_OVER_IF 0x00000001 /* RX Overflow Error Interrupt Flag */ > +#define PEF 0x00000010 /* Parity Error Flag*/ > +#define FEF 0x00000020 /* Framing Error Flag */ > +#define BIF 0x00000040 /* Break Interrupt Flag */ > +#define RX_EMPTY 0x00004000 /* Receiver FIFO Empty (Read Only) */ > +#define RX_FULL 0x00008000 /* Receiver FIFO Full (Read Only) */ > +#define TX_EMPTY 0x00400000 /* Transmitter FIFO Empty (Read Only) */ > +#define TX_FULL 0x00800000 /* Transmitter FIFO Full (Read Only) */ > +#define TX_OVER_IF 0x01000000 /* TX Overflow Error Interrupt Flag */ > +#define TE_FLAG 0x10000000 /* Transmitter Empty Flag (Read Only) */ > + > +/* UART Interrupt Status Register */ > +#define UART_REG_ISR 0x1C > +#define RDA_IF 0x00000001 /* RBR Available Interrupt Flag */ > +#define THRE_IF 0x00000002 /* THR Empty Interrupt Flag */ > +#define RLSIF 0x00000004 /* Receive Line Interrupt Flag */ > +#define MODEMIF 0x00000008 /* MODEM Interrupt Flag */ > +#define RXTO_IF 0x00000010 /* RX Time-out Interrupt Flag */ > +#define BUFEIF 0x00000020 /* Buffer Error Interrupt Flag */ > +#define WK_IF 0x00000040 /* UART Wake-up Interrupt Flag */ > +#define RDAINT 0x00000100 /* RBR Available Interrupt Indicator */ > +#define THRE_INT 0x00000200 /* THR Empty Interrupt Indicator */ > + > +/* UART Time-out Register */ > +#define UART_REG_TOR 0x20 > + > +/* UART Baud Rate Divider Register */ > +#define UART_REG_BAUD 0x24 > + > +/* UART Alternate Control/Status Register */ > +#define UART_REG_ALT_CSR 0x2C > + > +/* UART Function Select Register */ > +#define UART_FUN_SEL 0x30 > +#define FUN_SEL_UART 0x00000000 > +#define FUN_SEL_RS485 0x00000003 > +#define FUN_SEL_MASK 0x00000007 GENMASK(), then use FIELD_PREP() for the values. > + > +/* UART Wake-up Control Register */ > +#define UART_REG_WKCTL 0x40 > + > +/* UART Wake-up Status Register */ > +#define UART_REG_WKSTS 0x44 > + > +#endif /* __MA35D1_SERIAL_H__ */ > diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h > index 281fa286555c..c6d53db17042 100644 > --- a/include/uapi/linux/serial_core.h > +++ b/include/uapi/linux/serial_core.h > @@ -279,4 +279,7 @@ > /* Sunplus UART */ > #define PORT_SUNPLUS 123 > > +/* Nuvoton MA35D1 UART */ > +#define PORT_MA35D1 124 > + > #endif /* _UAPILINUX_SERIAL_CORE_H */ > -- i.