Re: [PATCH v10 10/10] tty: serial: Add Nuvoton ma35d1 serial driver support

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 27909 bytes --]

On Mon, 8 May 2023, Jacky Huang wrote:

> From: Jacky Huang <ychuang3@nuvoton.com>
> 
> This adds UART and console driver for Nuvoton ma35d1 Soc.
> It supports full-duplex communication, FIFO control, and
> hardware flow control.
> 
> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
> ---
>  drivers/tty/serial/Kconfig         |  18 +
>  drivers/tty/serial/Makefile        |   1 +
>  drivers/tty/serial/ma35d1_serial.c | 796 +++++++++++++++++++++++++++++
>  3 files changed, 815 insertions(+)
>  create mode 100644 drivers/tty/serial/ma35d1_serial.c
> 
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 398e5aac2e77..85ac6fc1ff1d 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1555,6 +1555,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_MA35 || 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..0e823851c42c 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..4eca34ba01c9
> --- /dev/null
> +++ b/drivers/tty/serial/ma35d1_serial.c
> @@ -0,0 +1,796 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  MA35D1 serial driver
> + *  Copyright (C) 2023 Nuvoton Technology Corp.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
> +#include <linux/tty_flip.h>
> +
> +#define UART_NR			17
> +
> +#define UART_REG_RBR		0x00
> +#define UART_REG_THR		0x00
> +#define UART_REG_IER		0x04
> +#define UART_REG_FCR		0x08
> +#define UART_REG_LCR		0x0C
> +#define UART_REG_MCR		0x10

These duplicate include/uapi/linux/serial_reg.h ones, use the std ones 
directly.

Setup regshift too and use it in serial_in.

> +#define UART_REG_MSR		0x14
> +#define UART_REG_FSR		0x18
> +#define UART_REG_ISR		0x1C
> +#define UART_REG_TOR		0x20
> +#define UART_REG_BAUD		0x24
> +#define UART_REG_ALTCTL		0x2C
> +#define UART_FUN_SEL		0x30
> +#define UART_REG_WKCTL		0x40
> +#define UART_REG_WKSTS		0x44

It would be better to prefix HW specific defines with something HW related 
such as MA35D1. This applies to most of the below ones too:

> +/* UART_REG_IER - Interrupt Enable Register */
> +#define IER_RDA_IEN		BIT(0)  /* RBR Available Interrupt Enable */
> +#define IER_THRE_IEN		BIT(1)  /* THR Empty Interrupt Enable */
> +#define IER_RLS_IEN		BIT(2)  /* RX Line Status Interrupt Enable */

These look same as UART_IER bits, use the std ones.

> +#define IER_RTO_IEN		BIT(4)  /* RX Time-out Interrupt Enable */
> +#define IER_BUFERR_IEN		BIT(5)  /* Buffer Error Interrupt Enable */
> +#define IER_TIME_OUT_EN		BIT(11) /* RX Buffer Time-out Counter Enable */
> +#define IER_AUTO_RTS		BIT(12) /* nRTS Auto-flow Control Enable */
> +#define IER_AUTO_CTS		BIT(13) /* nCTS Auto-flow Control Enable */
> +
> +/* UART_REG_FCR - FIFO Control Register */
> +#define FCR_RFR			BIT(1)  /* RX Field Software Reset */
> +#define FCR_TFR			BIT(2)  /* TX Field Software Reset */

Are these same as UART_FCR_CLEAR_* functionality wise? If they're use std 
ones.

> +#define FCR_RFITL_MASK		GENMASK(7, 4) /* RX FIFO Interrupt Trigger Level */
> +#define FCR_RFITL_1BYTE		FIELD_PREP(FCR_RFITL_MASK, 0)
> +#define FCR_RFITL_4BYTES	FIELD_PREP(FCR_RFITL_MASK, 1)
> +#define FCR_RFITL_8BYTES	FIELD_PREP(FCR_RFITL_MASK, 2)
> +#define FCR_RFITL_14BYTES	FIELD_PREP(FCR_RFITL_MASK, 3)
> +#define FCR_RFITL_30BYTES	FIELD_PREP(FCR_RFITL_MASK, 4)
> +#define FCR_RTSTRGLV_MASK	GENMASK(19, 16) /* nRTS Trigger Level */
> +#define FCR_RTSTRGLV_1BYTE	FIELD_PREP(FCR_RTSTRGLV_MASK, 0)
> +#define FCR_RTSTRGLV_4BYTES	FIELD_PREP(FCR_RTSTRGLV_MASK, 1)
> +#define FCR_RTSTRGLV_8BYTES	FIELD_PREP(FCR_RTSTRGLV_MASK, 2)
> +#define FCR_RTSTRGLV_14BYTES	FIELD_PREP(FCR_RTSTRGLV_MASK, 3)
> +#define FCR_RTSTRGLVL_30BYTES	FIELD_PREP(FCR_RTSTRGLV_MASK, 4)
> +
> +/* UART_REG_LCR - Line Control Register */
> +#define	LCR_NSB			BIT(2) /* Number of “STOP Bit” */
> +#define LCR_PBE			BIT(3) /* Parity Bit Enable */
> +#define LCR_EPE			BIT(4) /* Even Parity Enable */
> +#define LCR_SPE			BIT(5) /* Stick Parity Enable */
> +#define LCR_BREAK		BIT(6) /* Break Control */
> +#define LCR_WLS_MASK		GENMASK(1, 0) /* Word Length Selection */
> +#define LCR_WLS_5BITS		FIELD_PREP(LCR_WLS_MASK, 0)
> +#define LCR_WLS_6BITS		FIELD_PREP(LCR_WLS_MASK, 1)
> +#define LCR_WLS_7BITS		FIELD_PREP(LCR_WLS_MASK, 2)
> +#define LCR_WLS_8BITS		FIELD_PREP(LCR_WLS_MASK, 3)

These look exact copies of UART_LCR_* bits.

> +/* UART_REG_MCR - Modem Control Register */
> +#define MCR_RTS_CTRL		BIT(1)  /* nRTS Signal Control */

Is this same as UART_MCR_RTS?

> +#define MCR_RTSACTLV		BIT(9)  /* nRTS Pin Active Level */
> +#define MCR_RTSSTS		BIT(13) /* nRTS Pin Status (Read Only) */
> +
> +/* UART_REG_MSR - Modem Status Register */
> +#define MSR_CTSDETF		BIT(0)  /* Detect nCTS State Change Flag */
> +#define MSR_CTSSTS		BIT(4)  /* nCTS Pin Status (Read Only) */
> +#define MSR_CTSACTLV		BIT(8)  /* nCTS Pin Active Level */
> +
> +/* UART_REG_FSR - FIFO Status Register */
> +#define FSR_RX_OVER_IF		BIT(0)  /* RX Overflow Error Interrupt Flag */
> +#define FSR_PEF			BIT(4)  /* Parity Error Flag*/
> +#define FSR_FEF			BIT(5)  /* Framing Error Flag */
> +#define FSR_BIF			BIT(6)  /* Break Interrupt Flag */
> +#define FSR_RX_EMPTY		BIT(14) /* Receiver FIFO Empty (Read Only) */
> +#define FSR_RX_FULL		BIT(15) /* Receiver FIFO Full (Read Only) */
> +#define FSR_TX_EMPTY		BIT(22) /* Transmitter FIFO Empty (Read Only) */
> +#define FSR_TX_FULL		BIT(23) /* Transmitter FIFO Full (Read Only) */
> +#define FSR_TX_OVER_IF		BIT(24) /* TX Overflow Error Interrupt Flag */
> +#define FSR_TE_FLAG		BIT(28) /* Transmitter Empty Flag (Read Only) */
> +#define FSR_RXPTR_MSK		GENMASK(13, 8) /* TX FIFO Pointer mask */
> +#define FSR_TXPTR_MSK		GENMASK(21, 16) /* RX FIFO Pointer mask */
> +
> +/* UART_REG_ISR - Interrupt Status Register */
> +#define ISR_RDA_IF		BIT(0) /* RBR Available Interrupt Flag */
> +#define ISR_THRE_IF		BIT(1) /* THR Empty Interrupt Flag */
> +#define ISR_RLSIF		BIT(2) /* Receive Line Interrupt Flag */
> +#define ISR_MODEMIF		BIT(3) /* MODEM Interrupt Flag */
> +#define ISR_RXTO_IF		BIT(4) /* RX Time-out Interrupt Flag */
> +#define ISR_BUFEIF		BIT(5) /* Buffer Error Interrupt Flag */
> +#define UART_ISR_WK_IF		BIT(6) /* UART Wake-up Interrupt Flag */
> +#define UART_ISR_RDAINT		BIT(8) /* RBR Available Interrupt Indicator */
> +#define ISR_THRE_INT		BIT(9) /* THR Empty Interrupt Indicator */
> +#define ISR_ALL			0xFFFFFFFF
> +
> +/* UART_REG_BAUD - Baud Rate Divider Register */
> +#define	BAUD_MODE_MASK		GENMASK(29, 28)
> +#define BAUD_MODE0		FIELD_PREP(BAUD_MODE_MASK, 0)
> +#define BAUD_MODE1		FIELD_PREP(BAUD_MODE_MASK, 2)
> +#define BAUD_MODE2		FIELD_PREP(BAUD_MODE_MASK, 3)
> +
> +/* UART_REG_ALTCTL - Alternate Control/Status Register */
> +#define ALTCTL_RS485AUD		BIT(10) /* RS-485 Auto Direction Function */
> +
> +/* UART_FUN_SEL - Function Select Register */
> +#define FUN_SEL_MASK		GENMASK(2, 0)
> +#define FUN_SEL_UART		FIELD_PREP(FUN_SEL_MASK, 0)
> +#define FUN_SEL_RS485		FIELD_PREP(FUN_SEL_MASK, 3)
> +
> +/* UART FIFO depth */
> +#define UART_FIFO_DEPTH		32
> +/* UART console clock */
> +#define UART_CONSOLE_CLK	24000000

* HZ_PER_MHZ ?

> +/* UART register ioremap size */
> +#define UART_REG_SIZE		0x100
> +/* Rx Timeout */
> +#define UART_RX_TOUT		0x40
> +
> +#define UART_ISR_IF_CHECK (ISR_RDA_IF | ISR_RXTO_IF | ISR_THRE_INT | ISR_BUFEIF)
> +
> +#define LOOP_TIMEOUT		1000

Unused.

> +static struct uart_driver ma35d1serial_reg;
> +
> +struct uart_ma35d1_port {
> +	struct uart_port port;
> +	struct clk *clk;
> +	u16 capabilities; /* port capabilities */
> +	u8 ier;
> +	u8 lcr;
> +	u8 mcr;
> +	u32 baud_rate;
> +	u32 console_baud_rate;
> +	u32 console_line;
> +	u32 console_int;
> +};
> +
> +static struct uart_ma35d1_port ma35d1serial_ports[UART_NR];
> +
> +static struct uart_ma35d1_port *to_ma35d1_uart_port(struct uart_port *uart)
> +{
> +	return container_of(uart, struct uart_ma35d1_port, port);
> +}
> +
> +static u32 serial_in(struct uart_ma35d1_port *p, u32 offset)
> +{
> +	return readl_relaxed(p->port.membase + offset);
> +}
> +
> +static void serial_out(struct uart_ma35d1_port *p, u32 offset, u32 value)
> +{
> +	writel_relaxed(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 & IER_THRE_IEN)
> +		serial_out(p, UART_REG_IER, ier & ~IER_THRE_IEN);
> +}
> +
> +static void ma35d1serial_stop_tx(struct uart_port *port)
> +{
> +	struct uart_ma35d1_port *up = to_ma35d1_uart_port(port);
> +
> +	__stop_tx(up);
> +}
> +
> +static void transmit_chars(struct uart_ma35d1_port *up)
> +{
> +	u32 count;
> +	u8 ch;
> +
> +	if (uart_tx_stopped(&up->port)) {
> +		ma35d1serial_stop_tx(&up->port);
> +		return;
> +	}
> +	count = UART_FIFO_DEPTH - FIELD_GET(FSR_TXPTR_MSK, serial_in(up, UART_REG_FSR));
> +	uart_port_tx_limited(&up->port, ch, count,
> +			     !(serial_in(up, UART_REG_FSR) & FSR_TX_FULL),
> +			     serial_out(up, UART_REG_THR, ch),
> +			     ({}));
> +}
> +
> +static void ma35d1serial_start_tx(struct uart_port *port)
> +{
> +	struct uart_ma35d1_port *up = to_ma35d1_uart_port(port);
> +	u32 ier;
> +
> +	ier = serial_in(up, UART_REG_IER);
> +	serial_out(up, UART_REG_IER, ier & ~IER_THRE_IEN);
> +	transmit_chars(up);
> +	serial_out(up, UART_REG_IER, ier | IER_THRE_IEN);
> +}
> +
> +static void ma35d1serial_stop_rx(struct uart_port *port)
> +{
> +	struct uart_ma35d1_port *up = to_ma35d1_uart_port(port);
> +
> +	serial_out(up, UART_REG_IER, serial_in(up, UART_REG_IER) & ~IER_RDA_IEN);
> +}
> +
> +static void receive_chars(struct uart_ma35d1_port *up)
> +{
> +	u8 ch, flag;
> +	u32 fsr;
> +	int max_count = 256;
> +
> +	fsr = serial_in(up, UART_REG_FSR);
> +	do {
> +		flag = TTY_NORMAL;
> +		up->port.icount.rx++;
> +
> +		if (unlikely(fsr & (FSR_BIF | FSR_FEF | FSR_PEF | FSR_RX_OVER_IF))) {
> +			if (fsr & FSR_BIF) {
> +				up->port.icount.brk++;
> +				if (uart_handle_break(&up->port))
> +					continue;
> +			}
> +			if (fsr & FSR_FEF)
> +				up->port.icount.frame++;
> +			if (fsr & FSR_PEF)
> +				up->port.icount.parity++;
> +			if (fsr & FSR_RX_OVER_IF)
> +				up->port.icount.overrun++;
> +
> +			serial_out(up, UART_REG_FSR, fsr &
> +				   (FSR_BIF | FSR_FEF | FSR_PEF | FSR_RX_OVER_IF));
> +
> +			if (fsr & FSR_BIF)
> +				flag = TTY_BREAK;
> +			else if (fsr & FSR_PEF)
> +				flag = TTY_PARITY;
> +			else if (fsr & FSR_FEF)
> +				flag = TTY_FRAME;
> +		}
> +
> +		ch = serial_in(up, UART_REG_RBR);
> +		if (uart_handle_sysrq_char(&up->port, ch))
> +			continue;
> +
> +		spin_lock(&up->port.lock);
> +		uart_insert_char(&up->port, fsr, FSR_RX_OVER_IF, ch, flag);
> +		spin_unlock(&up->port.lock);
> +
> +		fsr = serial_in(up, UART_REG_FSR);
> +	} while (!(fsr & FSR_RX_EMPTY) && (max_count-- > 0));
> +
> +	spin_lock(&up->port.lock);
> +	tty_flip_buffer_push(&up->port.state->port);
> +	spin_unlock(&up->port.lock);
> +}
> +
> +static irqreturn_t ma35d1serial_interrupt(int irq, void *dev_id)
> +{
> +	struct uart_port *port = dev_id;
> +	struct uart_ma35d1_port *up = to_ma35d1_uart_port(port);
> +	u32 isr, fsr;
> +
> +	isr = serial_in(up, UART_REG_ISR);
> +	fsr = serial_in(up, UART_REG_FSR);
> +
> +	if (!(isr & UART_ISR_IF_CHECK))
> +		return IRQ_NONE;
> +
> +	if (isr & (ISR_RDA_IF | ISR_RXTO_IF))
> +		receive_chars(up);
> +	if (isr & ISR_THRE_INT)
> +		transmit_chars(up);
> +	if (fsr & FSR_TX_OVER_IF)
> +		serial_out(up, UART_REG_FSR, FSR_TX_OVER_IF);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static u32 ma35d1serial_tx_empty(struct uart_port *port)
> +{
> +	struct uart_ma35d1_port *up = to_ma35d1_uart_port(port);
> +	u32 fsr;
> +
> +	fsr = serial_in(up, UART_REG_FSR);
> +	return (fsr & (FSR_TE_FLAG | FSR_TX_EMPTY)) ==
> +		(FSR_TE_FLAG | FSR_TX_EMPTY) ? TIOCSER_TEMT : 0;
> +}
> +
> +static u32 ma35d1serial_get_mctrl(struct uart_port *port)
> +{
> +	struct uart_ma35d1_port *up = to_ma35d1_uart_port(port);
> +	u32 status;
> +	u32 ret = 0;
> +
> +	status = serial_in(up, UART_REG_MSR);
> +	if (!(status & MSR_CTSSTS))
> +		ret |= TIOCM_CTS;
> +	return ret;
> +}
> +
> +static void ma35d1serial_set_mctrl(struct uart_port *port, u32 mctrl)
> +{
> +	struct uart_ma35d1_port *up = to_ma35d1_uart_port(port);
> +	u32 mcr = 0;
> +	u32 ier = 0;
> +
> +	if (mctrl & TIOCM_RTS) {
> +		mcr = serial_in(up, UART_REG_MCR);
> +		mcr |= MCR_RTSACTLV;
> +		mcr &= ~MCR_RTS_CTRL;
> +	}
> +	if (up->mcr & UART_MCR_AFE) {
> +		mcr = serial_in(up, UART_REG_MCR);
> +		mcr |= MCR_RTSACTLV;
> +		mcr &= ~MCR_RTS_CTRL;
> +
> +		serial_out(up, UART_REG_IER,
> +			   (serial_in(up, UART_REG_IER) | IER_AUTO_RTS | IER_AUTO_CTS));

Split to 3 lines.

> +
> +		up->port.flags |= UPF_HARD_FLOW;
> +	} else {
> +		ier = serial_in(up, UART_REG_IER);
> +		ier &= ~(IER_AUTO_RTS | IER_AUTO_CTS);
> +		serial_out(up, UART_REG_IER, ier);
> +
> +		up->port.flags &= ~UPF_HARD_FLOW;
> +	}
> +	serial_out(up, UART_REG_MSR, (serial_in(up, UART_REG_MSR) | MSR_CTSACTLV));

Split this to 3 lines.

> +	serial_out(up, UART_REG_MCR, mcr);

Is it okay to write 0 to MCR here (when neither of the branches that read 
the previous value above are taken)?

> +}
> +
> +static void ma35d1serial_break_ctl(struct uart_port *port, int break_state)
> +{
> +	struct uart_ma35d1_port *up = to_ma35d1_uart_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 |= LCR_BREAK;
> +	else
> +		lcr &= ~LCR_BREAK;
> +	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 = to_ma35d1_uart_port(port);
> +	int retval;
> +
> +	/* Reset FIFO */
> +	serial_out(up, UART_REG_FCR, FCR_TFR | FCR_RFR);
> +
> +	/* Clear pending interrupts */
> +	serial_out(up, UART_REG_ISR, ISR_ALL);
> +
> +	retval = request_irq(port->irq, ma35d1serial_interrupt, 0,
> +			     dev_name(port->dev), port);
> +	if (retval) {
> +		dev_err(up->port.dev, "request irq failed.\n");
> +		return retval;
> +	}
> +
> +	serial_out(up, UART_REG_FCR, serial_in(up, UART_REG_FCR) |
> +		   FCR_RFITL_4BYTES | FCR_RTSTRGLV_8BYTES);
> +	serial_out(up, UART_REG_LCR, LCR_WLS_8BITS);
> +	serial_out(up, UART_REG_TOR, UART_RX_TOUT);
> +	serial_out(up, UART_REG_IER, IER_RTO_IEN | IER_RDA_IEN |
> +		   IER_TIME_OUT_EN | IER_BUFERR_IEN);
> +	return 0;
> +}
> +
> +static void ma35d1serial_shutdown(struct uart_port *port)
> +{
> +	struct uart_ma35d1_port *up = to_ma35d1_uart_port(port);
> +
> +	serial_out(up, UART_REG_IER, 0);
> +	free_irq(port->irq, port);
> +}
> +
> +static u32 ma35d1serial_get_divisor(struct uart_port *port, u32 baud)
> +{
> +	return (port->uartclk / baud) - 2;

What is the magic 2?

> +}
> +
> +static void ma35d1serial_set_termios(struct uart_port *port,
> +				     struct ktermios *termios,
> +				     const struct ktermios *old)
> +{
> +	struct uart_ma35d1_port *up = to_ma35d1_uart_port(port);
> +	u32 lcr = 0;
> +	unsigned long flags;
> +	u32 baud, quot;
> +
> +	lcr = UART_LCR_WLEN(tty_get_char_size(termios->c_cflag));
> +
> +	if (termios->c_cflag & CSTOPB)
> +		lcr |= LCR_NSB;
> +	if (termios->c_cflag & PARENB)
> +		lcr |= LCR_PBE;
> +	if (!(termios->c_cflag & PARODD))
> +		lcr |= LCR_EPE;
> +	if (termios->c_cflag & CMSPAR)
> +		lcr |= LCR_SPE;
> +
> +	baud = uart_get_baud_rate(port, termios, old, port->uartclk / 0xffff,

UART_DIV_MAX ?

> +				  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 = FSR_RX_OVER_IF;
> +	if (termios->c_iflag & INPCK)
> +		up->port.read_status_mask |= FSR_FEF | FSR_PEF;
> +	if (termios->c_iflag & (BRKINT | PARMRK))
> +		up->port.read_status_mask |= FSR_BIF;
> +
> +	/* Characteres to ignore */
> +	up->port.ignore_status_mask = 0;
> +	if (termios->c_iflag & IGNPAR)
> +		up->port.ignore_status_mask |= FSR_FEF | FSR_PEF;
> +	if (termios->c_iflag & IGNBRK) {
> +		up->port.ignore_status_mask |= FSR_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 |= FSR_RX_OVER_IF;
> +	}
> +	if (termios->c_cflag & CRTSCTS)
> +		up->mcr |= UART_MCR_AFE;
> +	else
> +		up->mcr &= ~UART_MCR_AFE;
> +
> +	uart_update_timeout(port, termios->c_cflag, baud);
> +	ma35d1serial_set_mctrl(&up->port, up->port.mctrl);
> +	serial_out(up, UART_REG_BAUD, quot | BAUD_MODE2);
> +	serial_out(up, UART_REG_LCR, lcr);
> +	spin_unlock_irqrestore(&up->port.lock, flags);
> +}
> +
> +static const char *ma35d1serial_type(struct uart_port *port)
> +{
> +	return "ma35d1-uart";
> +}
> +
> +static void ma35d1serial_config_port(struct uart_port *port, int flags)
> +{
> +	/*
> +	 * Driver core for serial ports forces a non-zero value for port type.
> +	 * Write an arbitrary value here to accommodate the serial core driver,
> +	 * as ID part of UAPI is redundant.
> +	 */
> +	port->type = 1;
> +}
> +
> +static int ma35d1serial_verify_port(struct uart_port *port,
> +				struct serial_struct *ser)
> +{
> +	if (port->type != PORT_UNKNOWN && ser->type != 1)
> +		return -EINVAL;
> +
> +	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,
> +	.config_port  = ma35d1serial_config_port,
> +	.verify_port  = ma35d1serial_verify_port,
> +};
> +
> +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 struct device_node *ma35d1serial_uart_nodes[UART_NR];
> +
> +static void wait_for_xmitr(struct uart_ma35d1_port *up)
> +{
> +	unsigned int tmout;
> +
> +	/* Wait up to 10ms for the character(s) to be sent. */
> +	tmout = 10000;
> +	while (--tmout) {
> +		if (serial_in(up, UART_REG_FSR) & FSR_TX_EMPTY)
> +			break;
> +		udelay(1);
> +	}
> +}
> +
> +static void ma35d1serial_console_putchar(struct uart_port *port, unsigned char ch)
> +{
> +	struct uart_ma35d1_port *up = to_ma35d1_uart_port(port);
> +
> +	wait_for_xmitr(up);
> +	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;
> +
> +	spin_lock_irqsave(&up->port.lock, 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);
> +
> +	wait_for_xmitr(up);
> +	serial_out(up, UART_REG_IER, ier);
> +
> +	spin_unlock_irqrestore(&up->port.lock, 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, UART_REG_SIZE);
> +	p->port.ops = &ma35d1serial_ops;
> +	p->port.line = 0;
> +	p->port.uartclk = UART_CONSOLE_CLK;
> +
> +	port = &ma35d1serial_ports[co->index].port;
> +
> +	if (options)
> +		uart_parse_options(options, &baud, &parity, &bits, &flow);
> +
> +	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)
> +{
> +	u32 i = 0;
> +	struct device_node *np;
> +
> +	for_each_matching_node(np, ma35d1_serial_of_match) {
> +		if (ma35d1serial_uart_nodes[i] == NULL) {
> +			of_node_get(np);
> +			ma35d1serial_uart_nodes[i] = np;
> +			i++;
> +			if (i == UART_NR)
> +				break;
> +		}
> +	}
> +}
> +
> +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,
> +};

This doesn't seem necessary, 8250 core will have the uart_driver for you
and most of the console stuff too. You just need to setup a few things 
correctly (see the setup functions in 8250_early for ideas/examples).

> +/*
> + * 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 = 0;
> +
> +	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);
> +			return ret;
> +		}
> +	}
> +	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, UART_REG_SIZE);
> +	up->port.ops = &ma35d1serial_ops;
> +
> +	spin_lock_init(&up->port.lock);
> +
> +	up->clk = of_clk_get(pdev->dev.of_node, 0);
> +	if (IS_ERR(up->clk)) {
> +		ret = PTR_ERR(up->clk);
> +		dev_err(&pdev->dev, "failed to get core clk: %d\n", ret);
> +		goto err_iounmap;
> +	}
> +
> +	ret = clk_prepare_enable(up->clk);
> +	if (ret)
> +		goto err_iounmap;
> +
> +	if (up->port.line != 0)
> +		up->port.uartclk = clk_get_rate(up->clk);
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0)
> +		goto err_clk_disable;
> +
> +	up->port.irq = ret;
> +	up->port.dev = &pdev->dev;
> +	up->port.flags = UPF_BOOT_AUTOCONF;
> +
> +	platform_set_drvdata(pdev, up);
> +
> +	ret = uart_add_one_port(&ma35d1serial_reg, &up->port);

For 8250, you should be using serial8250_register_8250_port(). See the 
other drivers how to setup the console functions.

> +	if (ret < 0)
> +		goto err_free_irq;
> +
> +	return 0;
> +
> +err_free_irq:
> +	free_irq(up->port.irq, &up->port);
> +
> +err_clk_disable:
> +	clk_disable_unprepare(up->clk);
> +
> +err_iounmap:
> +	iounmap(up->port.membase);
> +	return ret;
> +}
> +
> +/*
> + * Remove serial ports registered against a platform device.
> + */
> +static int ma35d1serial_remove(struct platform_device *dev)
> +{
> +	struct uart_port *port = platform_get_drvdata(dev);
> +	struct uart_ma35d1_port *up = to_ma35d1_uart_port(port);
> +
> +	uart_remove_one_port(&ma35d1serial_reg, port);

serial8250_unregister_port()

> +	clk_disable_unprepare(up->clk);
> +	return 0;
> +}
> +
> +static int ma35d1serial_suspend(struct platform_device *dev, pm_message_t state)
> +{
> +	struct uart_port *port = platform_get_drvdata(dev);
> +	struct uart_ma35d1_port *up = to_ma35d1_uart_port(port);
> +
> +	uart_suspend_port(&ma35d1serial_reg, &up->port);
> +	if (up->port.line == 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)
> +{
> +	struct uart_port *port = platform_get_drvdata(dev);
> +	struct uart_ma35d1_port *up = to_ma35d1_uart_port(port);
> +
> +	if (up->port.line == 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);
> +	}
> +	uart_resume_port(&ma35d1serial_reg, &up->port);
> +	return 0;
> +}
> +
> +static struct platform_driver ma35d1serial_driver = {
> +	.probe      = ma35d1serial_probe,
> +	.remove     = ma35d1serial_remove,

> +	.suspend    = ma35d1serial_suspend,
> +	.resume     = ma35d1serial_resume,

Shouldn't these be under .driver.pm instead?

> +	.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");
> +MODULE_DESCRIPTION("MA35D1 serial driver");
> +
> 

-- 
 i.