[14/15] tty: serial: Add Nuvoton ma35d1 serial driver support

From: Jacky Huang <ychuang3@nuvoton.com>

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.

Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
---
 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

Dear Jiri,


Thanks for your review.

I will create #define constant to replace every occurrence of magics or
hard coding number.

For every occurrence of "struct uart_ma35d1_port *up = (struct 
uart_ma35d1_port *)port; ",
it will be replaced with to_ma35d1_uart_port() .


On 2023/3/15 下午 06:13, Jiri Slaby wrote:
> On 15. 03. 23, 8:29, Jacky Huang wrote:
>> --- /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 <linux/module.h>
>> +#include <linux/moduleparam.h>
>
> What parameters does this module have?
+#include <linux/ioport.h>
>> +#include <linux/init.h>
>> +#include <linux/console.h>
>> +#include <linux/sysrq.h>
>> +#include <linux/delay.h>
>
> What do you use from delay.h?
>
>> +#include <linux/platform_device.h>
>> +#include <linux/tty.h>
>> +#include <linux/tty_flip.h>
>> +#include <linux/clk.h>
>> +#include <linux/serial_reg.h>
>> +#include <linux/serial_core.h>
>> +#include <linux/serial.h>
>> +#include <linux/nmi.h>
>
> nmi.h?
>
> Please clean up all of the includes.
>

moduleparam.h, delay.h, and nmi.h are not used.

I will test and remove all unused #include.

>> +#include <linux/mutex.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/io.h>
>> +#include <asm/irq.h>
>> +#include <asm/serial.h>
>> +#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 */
>
> Where are all those used?
>

I will remove the unused structure members in the next version.

>> +    struct serial_rs485 rs485; /* rs485 settings */
>> +    u32 baud_rate;
>
> And this one.
>

It's not used. I will remove it.

>> +    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);
>
> What for?

This function is used to disable TX FIFO empty interrupt.

naming issue?

>
>> +static void transmit_chars(struct uart_ma35d1_port *up);
>> +
>> +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, int offset)
>
> Q: int? A: No.

will modify "int offset" to "u32 offset"

>
>> +{
>> +    return __raw_readl(p->port.membase + offset);
>> +}
>> +
>> +static void serial_out(struct uart_ma35d1_port *p, int offset, int 
>> value)
>
> No ints here, please.

will modify offset and value data type to u32


>
>> +{
>> +    __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;
>
> Despite you have to_ma35d1_uart_port(), you do this?
>

You're right. I will use to_ma35d1_uart_port() to replace it.


>> +
>> +    __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)))
>
> You look like you need a helper for this computation (hint: 
> GENMASK()). What do those magic constants mean?
>

I will add #define macro for the bit mask..

>> +        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;
>
> Bah. Nah.

>> +
>> +    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;
>
> flag is u8 too. And a reverse xmas tree, please. Actually, you can put 
> all those u32 to a single line.
>

OK, I will fix it.

>> +
>> +    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++;
>> +            }
>> +            if (fsr & BIF)
>> +                flag = TTY_BREAK;
>> +            if (fsr & PEF)
>> +                flag = TTY_PARITY;
>> +            if (fsr & FEF)
>> +                flag = TTY_FRAME;
>> +        }
>> +        ch = (u8)serial_in(up, UART_REG_RBR);
>> +        if (uart_handle_sysrq_char(&up->port, ch))
>> +            continue;
>> +
>> +        uart_insert_char(&up->port, fsr, RX_OVER_IF, ch, flag);
>
> No lock needed?
>

I will wrap it with spin_lock and spin_unlock.

>> +        up->rx_count++;
>> +        dcnt = (serial_in(up, UART_REG_FSR) >> 8) & 0x3f;
>
> More magic constants. No.
>
>> +        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;
>> +            if ((isr & RXTO_IF) && (dcnt == 0))
>> +                goto tout_end;
>> +        }
>> +        if (isr & RDA_IF) {
>> +            if (dcnt == 1)
>> +                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)
>
> Why this cannot use uart_port_tx()?

OK, I will rename it as uart_port_tx().

>
>> +{
>> +    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));
>> +
>> +    return IRQ_HANDLED;
>
> You give no way for OS to disable the irq when the HW goes crazy. I.e. 
> you should return IRQ_HANDLED only when you really handled the irq.
>
>> +}
> ...
>> +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))
>
> 0x10 is magic.
>
>> +        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);
>
> This is repeated. Parentheses are superfluous. And again, 0x200, 0x2 
> are magic.
>
>> +
>> +        /* enable CTS/RTS auto-flow control */
>> +        serial_out(up, UART_REG_IER,
>> +               (serial_in(up, UART_REG_IER) | (0x3000)));
>> +
>> +        /* 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);
>
> Detto.
>
>> +        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 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 */);
>
> So why not RX_DIS?
>
>> +
>> +    /* Clear pending interrupts */
>> +    serial_out(up, UART_REG_ISR, 0xFFFFFFFF);
>> +
>> +    retval = request_irq(port->irq, ma35d1serial_interrupt, 0,
>> +                 tty ? tty->name : "ma35d1_serial", port);
>> +    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);
>
> You know what.
>
>> +    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);
>
> The two lines are switched, IMO. First disable HW, then let the ISR 
> finish and free it.

You're right. I will fix it.

>
>> +}
>> +
>> +static u32 ma35d1serial_get_divisor(struct uart_port *port, u32 baud)
>> +{
>> +    u32 quot;
>> +
>> +    quot = (port->uartclk / baud) - 2;
>> +    return quot;
>
> quot variable is completely superfluous.

remove quot
and
return (port->uartclk / baud) - 2;

>
>> +}
>> +
>> +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;
>> +    }
>
> IOW:
> lcr = UART_LCR_WLEN(tty_get_char_size(termios->c_cflag));
>
All registers in this uart controller are 32-bits.
lcr will finally be written to the register UART_REG_LCR.of
which bit[1:0] represents for UART word length.
00 - 5 bits
01 - 6 bits
10 - 7 bits
11 - 8bits

Instead, I will add
#define WLS_5    0x0
#define WLS_6    0x1
#define WLS_7    0x2
#define WLS_8    0x3

and replace lcr |= 1 with
lcr |= WLS_6;

>> +
>> +    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);
>> +    spin_unlock_irqrestore(&up->port.lock, flags);
>> +}
> ...
>> +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;
>
> ma35d1serial_request_port() does nothing. You can remove it altogether.
>

OK, I will remove it.

>> +    port->type = PORT_MA35D1;
>> +}
>
>
>> +static int ma35d1serial_ioctl(struct uart_port *port, u32 cmd, 
>> unsigned long arg)
>> +{
>> +    switch (cmd) {
>> +    default:
>> +        return -ENOIOCTLCMD;
>> +    }
>> +    return 0;
>> +}
>
> Drop that completely.
>

OK.

>> +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++;
>
> Unless the dt is broken, this is OK. But I would add a sanity check to i.
>

I will add
if (i == UART_NR)
     break;


>> +        }
>> +    }
>> +}
> ...
>> +/*
>> + * 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);
>> +        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, 0x10000);
>> +    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;
>> +    ret = uart_add_one_port(&ma35d1serial_reg, &up->port);
>
> What if this fails?
>

I will add return value check.

>> +    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);
>
> Hmm, this doesn't look right. You did that already, or?
>
>> +    for (i = 0; i < UART_NR; i++) {
>> +        struct uart_ma35d1_port *up = &ma35d1serial_ports[i];
>> +
>> +        if (up->port.dev == &dev->dev)
>
> You did platform_set_drvdata(), so why all this?
>

static int ma35d1serial_remove(struct platform_device *dev)
{
     struct uart_port *port = platform_get_drvdata(dev);

     uart_remove_one_port(&ma35d1serial_reg, &up->port);
     free_irq(port->irq, port);
     return 0;
}


>> + uart_remove_one_port(&ma35d1serial_reg, &up->port);
>> +    }
>> +    return 0;
>> +}
>


Best Regards,

Jacky Huang

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 <ychuang3@nuvoton.com>
> 
> 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 <ychuang3@nuvoton.com>
> ---
>  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 <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/ioport.h>
> +#include <linux/init.h>
> +#include <linux/console.h>
> +#include <linux/sysrq.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/clk.h>
> +#include <linux/serial_reg.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial.h>
> +#include <linux/nmi.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/io.h>
> +#include <asm/irq.h>
> +#include <asm/serial.h>
> +#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 */
>

On Mon, 20 Mar 2023, Jacky Huang wrote:

> Dear Ilpo,
> 
> 
> Thanks for your advice.
> 
> On 2023/3/16 下午 10:54, Ilpo Järvinen wrote:
> > 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 <ychuang3@nuvoton.com>
> > > 
> > > This adds UART and console driver for Nuvoton ma35d1 Soc.

> > > +		}
> > > +		ch = (u8)serial_in(up, UART_REG_RBR);
> > Drop the case.
> 
> I  will fix it.

I meant "cast" in case it wasn't obvious.

> > > +/* 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.
> 
> 
> OK, we will remove RS485 support from this initial patch.
> Once this initial patch was merged, we will submit the patch for RS485
> support.

You could do that but you could just as well include it into the same 
series as another patch after the main 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?
> 
> UART_FUN_SEL (2 bits) definition:
> 00 - UART function
> 01 - IrDA function
> 11 - RS485 function
> 
> The first searial_in() is used to clear set as UART function.
> The second one is used to set RS485 function if SER_RS485_ENABLED is true.

I got that, but it doesn't answer either of my questions which are:

Can you clear the UART function without causing a glitch in the RS485?
->rs485_config() can be called while already in RS485 mode so does it 
cause the UART to temporarily switch away from RS485 mode to "UART 
function" until the second write.

Also, you didn't explain why you need to read the register again, does 
the HW play with other bits when you do the clearing or to they remain 
the same (in which case you can just use a temporary variable to store 
the value)? ...It would be better to just write once too so this question 
might not matter in the end.

> > > +	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.
> 
> But the checkpatch limitation is 80 characters.

No, it isn't. It was changed years ago already.

> > > +++ 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 */

I forgot to mention earlier, there are many defines above which should use 
BIT().