[14/15] tty: serial: Add Nuvoton ma35d1 serial driver support
Commit Message
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
Comments
On Wed, Mar 15, 2023 at 07:29:01AM +0000, 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.
A new UART being designed that is not an 8250 compatible? Why????
Anyway, some minor comments:
> drivers/tty/serial/ma35d1_serial.c | 842 +++++++++++++++++++++++++++++
> drivers/tty/serial/ma35d1_serial.h | 93 ++++
Why do you have a .h file for only a single .c file? Please just put
them both together into one .c file.
> include/uapi/linux/serial_core.h | 3 +
Why do you need this #define? Are you using it in userspace now? If
not, why have it?
> +static void
> +receive_chars(struct uart_ma35d1_port *up)
Please just put all one one line.
> +{
> + 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)) {
You have no way out if the hardware is stuck? That feels wrong.
> +static int ma35d1serial_ioctl(struct uart_port *port, u32 cmd, unsigned long arg)
> +{
> + switch (cmd) {
> + default:
> + return -ENOIOCTLCMD;
> + }
> + return 0;
> +}
You do not need to handle any ioctls?
> +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));
Again, no way out if this gets stuck in a loop?
> +/**
> + * Suspend one serial port.
> + */
> +void ma35d1serial_suspend_port(int line)
> +{
> + uart_suspend_port(&ma35d1serial_reg, &ma35d1serial_ports[line].port);
> +}
> +EXPORT_SYMBOL(ma35d1serial_suspend_port);
Why is this exported? Who uses it?
And why not EXPORT_SYMBOL_GPL()?
> +
> +/**
> + * 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);
Same here, who calls this and why?
> --- 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
Again, why is this define needed?
thanks,
greg k-h
Dear Greg,
Thank you for your review.
On 2023/3/15 下午 03:37, Greg KH wrote:
> On Wed, Mar 15, 2023 at 07:29:01AM +0000, 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.
> A new UART being designed that is not an 8250 compatible? Why????
>
> Anyway, some minor comments:
This UART controller was designed for over 15 years ago and was used on
many Nuvoton chips.
The register interface is not compatible with 8250, so the 8250 driver
cannot be applied, but
the functions are compatible.
>> drivers/tty/serial/ma35d1_serial.c | 842 +++++++++++++++++++++++++++++
>> drivers/tty/serial/ma35d1_serial.h | 93 ++++
> Why do you have a .h file for only a single .c file? Please just put
> them both together into one .c file.
OK, we will put the .h into .c in the next version.
>> include/uapi/linux/serial_core.h | 3 +
> Why do you need this #define? Are you using it in userspace now? If
> not, why have it?
Actually, we do not use it from userspace. I will remove it in the next
version.
>> +static void
>> +receive_chars(struct uart_ma35d1_port *up)
> Please just put all one one line.
>
OK, I will fix it.
>> +{
>> + 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)) {
> You have no way out if the hardware is stuck? That feels wrong.
Thanks for pointing this out. I will add a timeout check to this
infinite loop.
>> +static int ma35d1serial_ioctl(struct uart_port *port, u32 cmd, unsigned long arg)
>> +{
>> + switch (cmd) {
>> + default:
>> + return -ENOIOCTLCMD;
>> + }
>> + return 0;
>> +}
> You do not need to handle any ioctls?
Yes, we do not handle ioctls.
I will remove both ma35d1serial_ioctl() and "ioctl =
ma35d1serial_ioctl," in the next version.
>> +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));
> Again, no way out if this gets stuck in a loop?
OK, we will fix it in the next version.
>> +/**
>> + * Suspend one serial port.
>> + */
>> +void ma35d1serial_suspend_port(int line)
>> +{
>> + uart_suspend_port(&ma35d1serial_reg, &ma35d1serial_ports[line].port);
>> +}
>> +EXPORT_SYMBOL(ma35d1serial_suspend_port);
> Why is this exported? Who uses it?
>
> And why not EXPORT_SYMBOL_GPL()?
>
>> +
>> +/**
>> + * 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);
> Same here, who calls this and why?
The ma35d1serial_suspend_port() and ma35d1serial_resume_port() were used in
previous ARM9 projects for userspace proprietary suspend/resume control.
As it's obsoleted in ma35s1, I will remove these two functions in the
next version.
>> --- 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
> Again, why is this define needed?
As replied above, we will remove the serial_core.h modification from
this patch.
> thanks,
>
> greg k-h
Best Regards,
Jacky Huang
Hi Jacky,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on clk/clk-next tty/tty-testing tty/tty-next tty/tty-linus linus/master v6.3-rc2 next-20230315]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jacky-Huang/arm64-Kconfig-platforms-Add-config-for-Nuvoton-MA35-platform/20230315-153355
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230315072902.9298-15-ychuang570808%40gmail.com
patch subject: [PATCH 14/15] tty: serial: Add Nuvoton ma35d1 serial driver support
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230315/202303151754.XvPyacT7-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/064028d2f2d911398012103aef3ce8666342ddfc
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jacky-Huang/arm64-Kconfig-platforms-Add-config-for-Nuvoton-MA35-platform/20230315-153355
git checkout 064028d2f2d911398012103aef3ce8666342ddfc
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/tty/serial/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303151754.XvPyacT7-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/tty/serial/ma35d1_serial.c:672:6: warning: no previous prototype for 'ma35d1serial_suspend_port' [-Wmissing-prototypes]
672 | void ma35d1serial_suspend_port(int line)
| ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/tty/serial/ma35d1_serial.c:681:6: warning: no previous prototype for 'ma35d1serial_resume_port' [-Wmissing-prototypes]
681 | void ma35d1serial_resume_port(int line)
| ^~~~~~~~~~~~~~~~~~~~~~~~
--
>> drivers/tty/serial/ma35d1_serial.c:670: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Suspend one serial port.
drivers/tty/serial/ma35d1_serial.c:679: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* Resume one serial port.
vim +/ma35d1serial_suspend_port +672 drivers/tty/serial/ma35d1_serial.c
668
669 /**
> 670 * Suspend one serial port.
671 */
> 672 void ma35d1serial_suspend_port(int line)
673 {
674 uart_suspend_port(&ma35d1serial_reg, &ma35d1serial_ports[line].port);
675 }
676 EXPORT_SYMBOL(ma35d1serial_suspend_port);
677
678 /**
679 * Resume one serial port.
680 */
> 681 void ma35d1serial_resume_port(int line)
682 {
683 struct uart_ma35d1_port *up = &ma35d1serial_ports[line];
684
685 uart_resume_port(&ma35d1serial_reg, &up->port);
686 }
687 EXPORT_SYMBOL(ma35d1serial_resume_port);
688
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.
> +#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?
> + struct serial_rs485 rs485; /* rs485 settings */
> + u32 baud_rate;
And this one.
> + 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?
> +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.
> +{
> + return __raw_readl(p->port.membase + offset);
> +}
> +
> +static void serial_out(struct uart_ma35d1_port *p, int offset, int value)
No ints here, please.
> +{
> + __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?
> +
> + __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?
> + 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.
> +
> + 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?
> + 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()?
> +{
> + 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.
> +}
> +
> +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.
> +}
> +
> +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));
> +
> + 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.
> + 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.
> +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.
> + }
> + }
> +}
...
> +/*
> + * 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?
> + 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?
> + uart_remove_one_port(&ma35d1serial_reg, &up->port);
> + }
> + return 0;
> +}
regards,
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 */
>
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.
>>
>> 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.
I will remove them and rewrite the descriptions.
>> 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.
OK, we will fix it.
>
>> +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));
> ?
OK, we will modify it clear flags together as possible.
> Also, add some driver specific prefix for the flag naming (all driver
> specific ones, not just these if you have others besides these).
>
We will fix it.
>> + 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?
We will modify it as:
if (fsr & BIF)
flag = TTY_BREAK;
else if (fsr & PEF)
flag = TTY_PARITY;
else if (fsr & FEF)
flag = TTY_FRAME;
>> + }
>> + ch = (u8)serial_in(up, UART_REG_RBR);
> Drop the case.
I will fix it.
>> + 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?
That is the tricky inherited from the previous arm9 project.
We will remove it.
>> + 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.
OK, We will fix it.
>> + 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??
Yes, most of these are redundant.
I will fix it as only
if (fsr & TX_OVER_IF))
serial_out(up, UART_REG_FSR, TX_OVER_IF);
>> +
>> + 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.
OK, we will drop these unused comments.
>> +
>> + /* 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).
We will rename it and remove comments.
>> + 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?
I will modify it as:
const char *name = to_platform_device(port->dev)->name;
retval = request_irq(port->irq, ma35d1serial_interrupt, 0, name, 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);
>> + 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.
We will fix it.
>
>> + 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.
OK, we will remove RS485 support from this initial patch.
Once this initial patch was merged, we will submit the patch for RS485
support.
>
>> + 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.
>> +
>> + 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.
But the checkpatch limitation is 80 characters.
>> + return ret;
> Misaligned line.
will be fixed
>
>> + }
>> + }
>> + 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.
OK, we will have #define for it.
>> + 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.
OK, we will add it.
>> + 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.
will be fixed
>
>> + 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.
I got it. Thank you.
>> +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.
We will fix them all.
>> +
>> +/* 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 */
>>
Best regards,
Jacky Huang
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().
Dear Ilpo,
On 2023/3/20 下午 06:04, Ilpo Järvinen wrote:
> 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.
I know that, thank you.
>>>> +/* 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.
Thank you for the detailed explanation.
OK, the register won't change. I will modify the code to read once and
write once only.
>>>> + 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.
I have a test on the checkpatch script.
You are right. It won't complain about over 80 characters now.
>>>> +++ 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().
>
Sure we will fix them all.
Best regards,
Jacky Huang
@@ -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
@@ -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
new file mode 100644
@@ -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);
+
+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)
+{
+ 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++;
+ }
+ 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);
+ 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;
+ 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)
+{
+ 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;
+}
+
+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)));
+
+ /* 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 */
+ 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);
+
+ 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);
+ 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;
+ 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);
+ 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;
+
+ 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));
+
+ 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);
+ 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);
+ 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);
+ 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);
+ 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");
+MODULE_DESCRIPTION("MA35D1 serial driver");
+MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
+
new file mode 100644
@@ -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
+
+/* UART Wake-up Control Register */
+#define UART_REG_WKCTL 0x40
+
+/* UART Wake-up Status Register */
+#define UART_REG_WKSTS 0x44
+
+#endif /* __MA35D1_SERIAL_H__ */
@@ -279,4 +279,7 @@
/* Sunplus UART */
#define PORT_SUNPLUS 123
+/* Nuvoton MA35D1 UART */
+#define PORT_MA35D1 124
+
#endif /* _UAPILINUX_SERIAL_CORE_H */