This is a patch series for UART driver for Suplus SP7021 SoC.
Sunplus SP7021 is an ARM Cortex A7 (4 cores) based SoC. It integrates
many peripherals (ex: UART. I2C, SPI, SDIO, eMMC, USB, SD card and
etc.) into a single chip. It is designed for industrial control.
Refer to:
https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview
https://tibbo.com/store/plus1.html
Hammer Hsieh (2):
dt-bindings:serial:Add bindings doc for Sunplus SoC UART Driver
serial:sunplus-uart:Add Sunplus SoC UART Driver
.../bindings/serial/sunplus,sp7021-uart.yaml | 58 ++
MAINTAINERS | 6 +
drivers/tty/serial/Kconfig | 23 +
drivers/tty/serial/Makefile | 1 +
drivers/tty/serial/sunplus-uart.c | 848 +++++++++++++++++++++
5 files changed, 936 insertions(+)
create mode 100644 Documentation/devicetree/bindings/serial/sunplus,sp7021-uart.yaml
create mode 100644 drivers/tty/serial/sunplus-uart.c
--
2.7.4
Add bindings doc for Sunplus SoC UART Driver
Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: Hammer Hsieh <[email protected]>
---
Changes in v4:
- no change.
- Reviewed-by : Rob Herring <[email protected]> in v3.
.../bindings/serial/sunplus,sp7021-uart.yaml | 58 ++++++++++++++++++++++
MAINTAINERS | 5 ++
2 files changed, 63 insertions(+)
create mode 100644 Documentation/devicetree/bindings/serial/sunplus,sp7021-uart.yaml
diff --git a/Documentation/devicetree/bindings/serial/sunplus,sp7021-uart.yaml b/Documentation/devicetree/bindings/serial/sunplus,sp7021-uart.yaml
new file mode 100644
index 0000000..df11074
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/sunplus,sp7021-uart.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) Sunplus Co., Ltd. 2021
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/serial/sunplus,sp7021-uart.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Sunplus SoC SP7021 UART Controller Device Tree Bindings
+
+maintainers:
+ - Tony Huang <[email protected]>
+ - Hammer Hsieh <[email protected]>
+ - Wells Lu <[email protected]>
+
+allOf:
+ - $ref: serial.yaml#
+
+properties:
+ compatible:
+ const: sunplus,sp7021-uart
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ resets:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - resets
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ aliases {
+ serial0 = &uart0;
+ };
+
+ uart0: serial@9c000900 {
+ compatible = "sunplus,sp7021-uart";
+ reg = <0x9c000900 0x80>;
+ interrupt-parent = <&intc>;
+ interrupts = <53 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clkc 0x28>;
+ resets = <&rstc 0x18>;
+ };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 3b79fd4..f2ee40c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17945,6 +17945,11 @@ L: [email protected]
S: Maintained
F: drivers/net/ethernet/dlink/sundance.c
+SUNPLUS UART DRIVER
+M: Hammer Hsieh <[email protected]>
+S: Maintained
+F: Documentation/devicetree/bindings/serial/sunplus,sp7021-uart.yaml
+
SUPERH
M: Yoshinori Sato <[email protected]>
M: Rich Felker <[email protected]>
--
2.7.4
Add Sunplus SoC UART Driver
Signed-off-by: Hammer Hsieh <[email protected]>
---
Changes in v4:
- Addressed all comments from Andy Shevchenko except pm runtime function.
MAINTAINERS | 1 +
drivers/tty/serial/Kconfig | 23 ++
drivers/tty/serial/Makefile | 1 +
drivers/tty/serial/sunplus-uart.c | 848 ++++++++++++++++++++++++++++++++++++++
4 files changed, 873 insertions(+)
create mode 100644 drivers/tty/serial/sunplus-uart.c
diff --git a/MAINTAINERS b/MAINTAINERS
index f2ee40c..65681ff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17949,6 +17949,7 @@ SUNPLUS UART DRIVER
M: Hammer Hsieh <[email protected]>
S: Maintained
F: Documentation/devicetree/bindings/serial/sunplus,sp7021-uart.yaml
+F: drivers/tty/serial/sunplus-uart.c
SUPERH
M: Yoshinori Sato <[email protected]>
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 131a6a5..ffdcf40 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1561,6 +1561,29 @@ config SERIAL_LITEUART_CONSOLE
and warnings and which allows logins in single user mode).
Otherwise, say 'N'.
+config SERIAL_SUNPLUS
+ tristate "Sunplus UART support"
+ depends on OF || COMPILE_TEST
+ select SERIAL_CORE
+ help
+ Select this option if you would like to use Sunplus serial port on
+ Sunplus SoC SP7021.
+ If you enable this option, Sunplus serial ports in the system will
+ be registered as ttySUPx.
+
+config SERIAL_SUNPLUS_CONSOLE
+ bool "Console on Sunplus UART"
+ depends on SERIAL_SUNPLUS
+ select SERIAL_CORE_CONSOLE
+ select SERIAL_EARLYCON
+ help
+ Select this option if you would like to use a Sunplus UART as the
+ system console.
+ Even if you say Y here, the currently visible virtual console
+ (/dev/tty0) will still be used as the system console by default, but
+ you can alter that using a kernel command line option such as
+ "console=ttySUPx".
+
endmenu
config SERIAL_MCTRL_GPIO
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index 7da0856..61cc8de 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -87,6 +87,7 @@ obj-$(CONFIG_SERIAL_RDA) += rda-uart.o
obj-$(CONFIG_SERIAL_MILBEAUT_USIO) += milbeaut_usio.o
obj-$(CONFIG_SERIAL_SIFIVE) += sifive.o
obj-$(CONFIG_SERIAL_LITEUART) += liteuart.o
+obj-$(CONFIG_SERIAL_SUNPLUS) += sunplus-uart.o
# GPIOLIB helpers for modem control lines
obj-$(CONFIG_SERIAL_MCTRL_GPIO) += serial_mctrl_gpio.o
diff --git a/drivers/tty/serial/sunplus-uart.c b/drivers/tty/serial/sunplus-uart.c
new file mode 100644
index 0000000..9395207
--- /dev/null
+++ b/drivers/tty/serial/sunplus-uart.c
@@ -0,0 +1,848 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Sunplus SoC UART driver
+ *
+ * Author: Hammer Hsieh <[email protected]>
+ * Tony Huang <[email protected]>
+ * Wells Lu <[email protected]>
+ */
+#include <linux/clk.h>
+#include <linux/console.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+#include <linux/serial_core.h>
+#include <linux/sysrq.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+#include <asm/irq.h>
+
+/* Register offsets */
+#define SUP_UART_DATA 0x00
+#define SUP_UART_LSR 0x04
+#define SUP_UART_MSR 0x08
+#define SUP_UART_LCR 0x0C
+#define SUP_UART_MCR 0x10
+#define SUP_UART_DIV_L 0x14
+#define SUP_UART_DIV_H 0x18
+#define SUP_UART_ISC 0x1C
+#define SUP_UART_TX_RESIDUE 0x20
+#define SUP_UART_RX_RESIDUE 0x24
+
+/* Line Status Register bits */
+#define SUP_UART_LSR_TXE BIT(6) /* tx empty */
+#define SUP_UART_LSR_BC BIT(5) /* break condition status */
+#define SUP_UART_LSR_FE BIT(4) /* frame error status */
+#define SUP_UART_LSR_OE BIT(3) /* overrun error status */
+#define SUP_UART_LSR_PE BIT(2) /* parity error status */
+#define SUP_UART_LSR_RX BIT(1) /* 1: receive fifo not empty */
+#define SUP_UART_LSR_TX BIT(0) /* 1: transmit fifo is not full */
+#define SUP_UART_LSR_TX_NOT_FULL 1
+#define SUP_UART_LSR_BRK_ERROR_BITS GENMASK(5, 2)
+
+/* Line Control Register bits */
+#define SUP_UART_LCR_BC BIT(5) /* break condition select */
+#define SUP_UART_LCR_PR BIT(4) /* parity bit polarity select */
+#define SUP_UART_LCR_PE BIT(3) /* parity bit enable */
+#define SUP_UART_LCR_ST BIT(2) /* stop bits select */
+#define SUP_UART_LCR_WL5 0x00 /* word length 5 */
+#define SUP_UART_LCR_WL6 0x01 /* word length 6 */
+#define SUP_UART_LCR_WL7 0x02 /* word length 7 */
+#define SUP_UART_LCR_WL8 0x03 /* word length 8 (default) */
+
+/* Modem Control Register bits */
+#define SUP_UART_MCR_LB BIT(4) /* Loopback mode */
+#define SUP_UART_MCR_RI BIT(3) /* ring indicator */
+#define SUP_UART_MCR_DCD BIT(2) /* data carrier detect */
+#define SUP_UART_MCR_RTS BIT(1) /* request to send */
+#define SUP_UART_MCR_DTS BIT(0) /* data terminal ready */
+
+/* Interrupt Status/Control Register bits */
+#define SUP_UART_ISC_RXM BIT(5) /* RX interrupt enable */
+#define SUP_UART_ISC_TXM BIT(4) /* TX interrupt enable */
+#define SUP_UART_ISC_RX BIT(1) /* RX interrupt status */
+#define SUP_UART_ISC_TX BIT(0) /* TX interrupt status */
+
+#define SUP_DUMMY_READ BIT(16) /* drop bytes received on a !CREAD port */
+#define UART_AUTOSUSPEND_TIMEOUT 3000
+#define SUP_UART_NR 5
+
+struct sunplus_uart_port {
+ struct uart_port port;
+ struct clk *clk;
+ struct reset_control *rstc;
+};
+
+static inline struct sunplus_uart_port *to_sunplus_uart(struct uart_port *port)
+{
+ return container_of(port, struct sunplus_uart_port, port);
+}
+
+static inline void sp_uart_put_char(struct uart_port *port, unsigned int ch)
+{
+ writel(ch, port->membase + SUP_UART_DATA);
+}
+
+static inline u32 sunplus_tx_buf_not_full(struct uart_port *port)
+{
+ unsigned int lsr = readl(port->membase + SUP_UART_LSR);
+
+ return ((lsr & SUP_UART_LSR_TX) ? SUP_UART_LSR_TX_NOT_FULL : 0);
+}
+
+static unsigned int sunplus_tx_empty(struct uart_port *port)
+{
+ unsigned int lsr = readl(port->membase + SUP_UART_LSR);
+
+ return ((lsr & SUP_UART_LSR_TXE) ? TIOCSER_TEMT : 0);
+}
+
+static void sunplus_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+ unsigned int mcr = readl(port->membase + SUP_UART_MCR);
+
+ if (mctrl & TIOCM_DTR)
+ mcr |= SUP_UART_MCR_DTS;
+ else
+ mcr &= ~SUP_UART_MCR_DTS;
+
+ if (mctrl & TIOCM_RTS)
+ mcr |= SUP_UART_MCR_RTS;
+ else
+ mcr &= ~SUP_UART_MCR_RTS;
+
+ if (mctrl & TIOCM_CAR)
+ mcr |= SUP_UART_MCR_DCD;
+ else
+ mcr &= ~SUP_UART_MCR_DCD;
+
+ if (mctrl & TIOCM_RI)
+ mcr |= SUP_UART_MCR_RI;
+ else
+ mcr &= ~SUP_UART_MCR_RI;
+
+ if (mctrl & TIOCM_LOOP)
+ mcr |= SUP_UART_MCR_LB;
+ else
+ mcr &= ~SUP_UART_MCR_LB;
+
+ writel(mcr, port->membase + SUP_UART_MCR);
+}
+
+static unsigned int sunplus_get_mctrl(struct uart_port *port)
+{
+ unsigned int ret, mcr;
+
+ mcr = readl(port->membase + SUP_UART_MCR);
+
+ if (mcr & SUP_UART_MCR_DTS)
+ ret |= TIOCM_DTR;
+
+ if (mcr & SUP_UART_MCR_RTS)
+ ret |= TIOCM_RTS;
+
+ if (mcr & SUP_UART_MCR_DCD)
+ ret |= TIOCM_CAR;
+
+ if (mcr & SUP_UART_MCR_RI)
+ ret |= TIOCM_RI;
+
+ if (mcr & SUP_UART_MCR_LB)
+ ret |= TIOCM_LOOP;
+
+ return ret;
+}
+
+static void sunplus_stop_tx(struct uart_port *port)
+{
+ unsigned int isc;
+
+ isc = readl(port->membase + SUP_UART_ISC);
+ isc &= ~SUP_UART_ISC_TXM;
+ writel(isc, port->membase + SUP_UART_ISC);
+}
+
+static void sunplus_start_tx(struct uart_port *port)
+{
+ unsigned int isc;
+
+ isc = readl(port->membase + SUP_UART_ISC);
+ isc |= SUP_UART_ISC_TXM;
+ writel(isc, port->membase + SUP_UART_ISC);
+}
+
+static void sunplus_stop_rx(struct uart_port *port)
+{
+ unsigned int isc;
+
+ isc = readl(port->membase + SUP_UART_ISC);
+ isc &= ~SUP_UART_ISC_RXM;
+ writel(isc, port->membase + SUP_UART_ISC);
+}
+
+static void sunplus_break_ctl(struct uart_port *port, int ctl)
+{
+ unsigned long flags;
+ unsigned int lcr;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ lcr = readl(port->membase + SUP_UART_LCR);
+
+ if (ctl)
+ lcr |= SUP_UART_LCR_BC; /* start break */
+ else
+ lcr &= ~SUP_UART_LCR_BC; /* stop break */
+
+ writel(lcr, port->membase + SUP_UART_LCR);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static void transmit_chars(struct uart_port *port)
+{
+ struct circ_buf *xmit = &port->state->xmit;
+
+ if (port->x_char) {
+ sp_uart_put_char(port, port->x_char);
+ port->icount.tx++;
+ port->x_char = 0;
+ return;
+ }
+
+ if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
+ sunplus_stop_tx(port);
+ return;
+ }
+
+ do {
+ sp_uart_put_char(port, xmit->buf[xmit->tail]);
+ xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
+ port->icount.tx++;
+
+ if (uart_circ_empty(xmit))
+ break;
+ } while (sunplus_tx_buf_not_full(port));
+
+ if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(port);
+
+ if (uart_circ_empty(xmit))
+ sunplus_stop_tx(port);
+}
+
+static void receive_chars(struct uart_port *port)
+{
+ unsigned int lsr = readl(port->membase + SUP_UART_LSR);
+ unsigned int ch, flag;
+
+ do {
+ ch = readl(port->membase + SUP_UART_DATA);
+ flag = TTY_NORMAL;
+ port->icount.rx++;
+
+ if (unlikely(lsr & SUP_UART_LSR_BRK_ERROR_BITS)) {
+ if (lsr & SUP_UART_LSR_BC) {
+ lsr &= ~(SUP_UART_LSR_FE | SUP_UART_LSR_PE);
+ port->icount.brk++;
+ if (uart_handle_break(port))
+ goto ignore_char;
+ } else if (lsr & SUP_UART_LSR_PE) {
+ port->icount.parity++;
+ } else if (lsr & SUP_UART_LSR_FE) {
+ port->icount.frame++;
+ }
+
+ if (lsr & SUP_UART_LSR_OE)
+ port->icount.overrun++;
+
+ if (lsr & SUP_UART_LSR_BC)
+ flag = TTY_BREAK;
+ else if (lsr & SUP_UART_LSR_PE)
+ flag = TTY_PARITY;
+ else if (lsr & SUP_UART_LSR_FE)
+ flag = TTY_FRAME;
+ }
+
+ if (port->ignore_status_mask & SUP_DUMMY_READ)
+ goto ignore_char;
+
+ if (uart_handle_sysrq_char(port, ch))
+ goto ignore_char;
+
+ uart_insert_char(port, lsr, SUP_UART_LSR_OE, ch, flag);
+
+ignore_char:
+ lsr = readl(port->membase + SUP_UART_LSR);
+ } while (lsr & SUP_UART_LSR_RX);
+
+ tty_flip_buffer_push(&port->state->port);
+}
+
+static irqreturn_t sunplus_uart_irq(int irq, void *args)
+{
+ struct uart_port *port = (struct uart_port *)args;
+ unsigned long flags;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ if (readl(port->membase + SUP_UART_ISC) & SUP_UART_ISC_RX)
+ receive_chars(port);
+
+ if (readl(port->membase + SUP_UART_ISC) & SUP_UART_ISC_TX)
+ transmit_chars(port);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ return IRQ_HANDLED;
+}
+
+static int sunplus_startup(struct uart_port *port)
+{
+ unsigned int isc;
+ int ret;
+
+#ifdef CONFIG_PM
+ if (!uart_console(port)) {
+ ret = pm_runtime_get_sync(port->dev);
+ if (ret < 0)
+ goto out;
+ }
+#endif
+ ret = request_irq(port->irq, sunplus_uart_irq, 0, "sunplus_uart", port);
+ if (ret)
+ return ret;
+
+ spin_lock_irq(&port->lock);
+
+ isc |= SUP_UART_ISC_RXM;
+ writel(isc, port->membase + SUP_UART_ISC);
+
+ spin_unlock_irq(&port->lock);
+
+#ifdef CONFIG_PM
+ if (!uart_console(port))
+ pm_runtime_put(port->dev);
+
+ return 0;
+out:
+ if (!uart_console(port)) {
+ pm_runtime_mark_last_busy(port->dev);
+ pm_runtime_put_autosuspend(port->dev);
+ }
+#endif
+ return 0;
+}
+
+static void sunplus_shutdown(struct uart_port *port)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&port->lock, flags);
+ writel(0, port->membase + SUP_UART_ISC);
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ free_irq(port->irq, port);
+}
+
+static void sunplus_set_termios(struct uart_port *port,
+ struct ktermios *termios,
+ struct ktermios *oldtermios)
+{
+ u32 clk, ext, div, div_l, div_h, baud;
+ u32 lcr, val;
+ unsigned long flags;
+
+ clk = port->uartclk;
+
+ baud = uart_get_baud_rate(port, termios, oldtermios, 0, port->uartclk / 16);
+
+ readl_poll_timeout_atomic(port->membase + SUP_UART_LSR, val,
+ (val & SUP_UART_LSR_TXE), 1, 10000);
+ /*
+ * baud rate = uartclk / ((16 * div[15:0] + 1) + div_ext[3:0])
+ * get target baud rate and uartclk
+ * auto calculate div and div_ext
+ * div_h = (div[15:8] >> 8); div_l = (div_ext[3:0] << 12) + div[7:0]
+ */
+ clk += baud >> 1;
+ div = clk / baud;
+ ext = div & 0x0F;
+ div = (div >> 4) - 1;
+ div_l = (div & 0xFF) | (ext << 12);
+ div_h = div >> 8;
+
+ switch (termios->c_cflag & CSIZE) {
+ case CS5:
+ lcr = SUP_UART_LCR_WL5;
+ break;
+ case CS6:
+ lcr = SUP_UART_LCR_WL6;
+ break;
+ case CS7:
+ lcr = SUP_UART_LCR_WL7;
+ break;
+ default: /* CS8 */
+ lcr = SUP_UART_LCR_WL8;
+ break;
+ }
+
+ if (termios->c_cflag & CSTOPB)
+ lcr |= SUP_UART_LCR_ST;
+
+ if (termios->c_cflag & PARENB) {
+ lcr |= SUP_UART_LCR_PE;
+
+ if (!(termios->c_cflag & PARODD))
+ lcr |= SUP_UART_LCR_PR;
+ }
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ uart_update_timeout(port, termios->c_cflag, baud);
+
+ port->read_status_mask = 0;
+ if (termios->c_iflag & INPCK)
+ port->read_status_mask |= SUP_UART_LSR_PE | SUP_UART_LSR_FE;
+
+ if (termios->c_iflag & (BRKINT | PARMRK))
+ port->read_status_mask |= SUP_UART_LSR_BC;
+
+ /* Characters to ignore */
+ port->ignore_status_mask = 0;
+ if (termios->c_iflag & IGNPAR)
+ port->ignore_status_mask |= SUP_UART_LSR_FE | SUP_UART_LSR_PE;
+
+ if (termios->c_iflag & IGNBRK) {
+ port->ignore_status_mask |= SUP_UART_LSR_BC;
+
+ if (termios->c_iflag & IGNPAR)
+ port->ignore_status_mask |= SUP_UART_LSR_OE;
+ }
+
+ /* Ignore all characters if CREAD is not set */
+ if ((termios->c_cflag & CREAD) == 0) {
+ port->ignore_status_mask |= SUP_DUMMY_READ;
+ /* flush rx data FIFO */
+ writel(0, port->membase + SUP_UART_RX_RESIDUE);
+ }
+
+ /* Settings for baud rate divisor and lcr */
+ writel(div_h, port->membase + SUP_UART_DIV_H);
+ writel(div_l, port->membase + SUP_UART_DIV_L);
+ writel(lcr, port->membase + SUP_UART_LCR);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static void sunplus_set_ldisc(struct uart_port *port, struct ktermios *termios)
+{
+ int new = termios->c_line;
+
+ if (new == N_PPS)
+ port->flags |= UPF_HARDPPS_CD;
+ else
+ port->flags &= ~UPF_HARDPPS_CD;
+}
+
+static const char *sunplus_type(struct uart_port *port)
+{
+ struct sunplus_uart_port *sup = to_sunplus_uart(port);
+
+ return sup->port.type == PORT_SUNPLUS ? "sunplus_uart" : NULL;
+}
+
+static void sunplus_release_port(struct uart_port *port)
+{
+}
+
+static int sunplus_request_port(struct uart_port *port)
+{
+ return 0;
+}
+
+static void sunplus_config_port(struct uart_port *port, int type)
+{
+ if (type & UART_CONFIG_TYPE) {
+ port->type = PORT_SUNPLUS;
+ sunplus_request_port(port);
+ }
+}
+
+static int sunplus_verify_port(struct uart_port *port, struct serial_struct *serial)
+{
+ if (serial->type != PORT_UNKNOWN && serial->type != PORT_SUNPLUS)
+ return -EINVAL;
+
+ return 0;
+}
+
+#ifdef CONFIG_CONSOLE_POLL
+static int sunplus_poll_init(struct uart_port *port)
+{
+ return 0;
+}
+
+static void sunplus_poll_put_char(struct uart_port *port, unsigned char data)
+{
+ wait_for_xmitr(port);
+ sp_uart_put_char(port, data);
+}
+
+static int sunplus_poll_get_char(struct uart_port *port)
+{
+ unsigned int lsr = readl(port->membase + SUP_UART_LSR);
+
+ if (!(lsr & SUP_UART_LSR_RX))
+ return NO_POLL_CHAR;
+
+ return readl(port->membase + SUP_UART_DATA);
+}
+#endif
+
+static const struct uart_ops sunplus_uart_ops = {
+ .tx_empty = sunplus_tx_empty,
+ .set_mctrl = sunplus_set_mctrl,
+ .get_mctrl = sunplus_get_mctrl,
+ .stop_tx = sunplus_stop_tx,
+ .start_tx = sunplus_start_tx,
+ .stop_rx = sunplus_stop_rx,
+ .break_ctl = sunplus_break_ctl,
+ .startup = sunplus_startup,
+ .shutdown = sunplus_shutdown,
+ .set_termios = sunplus_set_termios,
+ .set_ldisc = sunplus_set_ldisc,
+ .type = sunplus_type,
+ .release_port = sunplus_release_port,
+ .request_port = sunplus_request_port,
+ .config_port = sunplus_config_port,
+ .verify_port = sunplus_verify_port,
+#ifdef CONFIG_CONSOLE_POLL
+ .poll_init = sunplus_poll_init,
+ .poll_put_char = sunplus_poll_put_char,
+ .poll_get_char = sunplus_poll_get_char,
+#endif
+};
+
+#ifdef CONFIG_SERIAL_SUNPLUS_CONSOLE
+struct sunplus_uart_port *sunplus_console_ports[SUP_UART_NR];
+
+static inline void wait_for_xmitr(struct uart_port *port)
+{
+ unsigned int val;
+
+ readl_poll_timeout_atomic(port->membase + SUP_UART_LSR, val,
+ (val & SUP_UART_LSR_TX), 1, 10000);
+}
+
+static void sunplus_uart_console_putchar(struct uart_port *port, int ch)
+{
+ wait_for_xmitr(port);
+ sp_uart_put_char(port, ch);
+}
+
+static void sunplus_uart_console_write(struct console *co,
+ const char *s,
+ unsigned int count)
+{
+ unsigned long flags;
+ int locked = 1;
+
+ local_irq_save(flags);
+
+ if (sunplus_console_ports[co->index]->port.sysrq)
+ locked = 0;
+ else if (oops_in_progress)
+ locked = spin_trylock(&sunplus_console_ports[co->index]->port.lock);
+ else
+ spin_lock(&sunplus_console_ports[co->index]->port.lock);
+
+ uart_console_write(&sunplus_console_ports[co->index]->port, s, count,
+ sunplus_uart_console_putchar);
+
+ if (locked)
+ spin_unlock(&sunplus_console_ports[co->index]->port.lock);
+
+ local_irq_restore(flags);
+}
+
+static int __init sunplus_uart_console_setup(struct console *co, char *options)
+{
+ struct sunplus_uart_port *sup;
+ int baud = 115200;
+ int bits = 8;
+ int parity = 'n';
+ int flow = 'n';
+
+ if (co->index < 0 || co->index >= SUP_UART_NR)
+ return -EINVAL;
+
+ sup = sunplus_console_ports[co->index];
+
+ if (options)
+ uart_parse_options(options, &baud, &parity, &bits, &flow);
+
+ return uart_set_options(&sup->port, co, baud, parity, bits, flow);
+}
+
+static struct uart_driver sunplus_uart_driver;
+static struct console sunplus_uart_console = {
+ .name = "ttySUP",
+ .write = sunplus_uart_console_write,
+ .device = uart_console_device,
+ .setup = sunplus_uart_console_setup,
+ .flags = CON_PRINTBUFFER,
+ .index = -1,
+ .data = &sunplus_uart_driver
+};
+#endif
+
+static struct uart_driver sunplus_uart_driver = {
+ .owner = THIS_MODULE,
+ .driver_name = "sunplus_uart",
+ .dev_name = "ttySUP",
+ .major = TTY_MAJOR,
+ .minor = 64,
+ .nr = SUP_UART_NR,
+ .cons = NULL,
+};
+
+static int sunplus_uart_probe(struct platform_device *pdev)
+{
+ struct sunplus_uart_port *sup;
+ struct resource *res;
+ struct uart_port *port;
+ int ret, irq;
+
+ pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");
+ if (pdev->id < 0)
+ pdev->id = of_alias_get_id(pdev->dev.of_node, "uart");
+
+ if (pdev->id < 0 || pdev->id >= SUP_UART_NR)
+ return -EINVAL;
+
+ sup = devm_kzalloc(&pdev->dev, sizeof(*sup), GFP_KERNEL);
+ if (!sup)
+ return -ENOMEM;
+
+ sup->clk = devm_clk_get(&pdev->dev, NULL);
+ if (!IS_ERR(sup->clk)) {
+ ret = clk_prepare_enable(sup->clk);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(&pdev->dev,
+ (void(*)(void *))clk_disable_unprepare,
+ sup->clk);
+ if (ret)
+ return ret;
+ } else {
+ if (PTR_ERR(sup->clk) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ return dev_err_probe(&pdev->dev, PTR_ERR(sup->clk), "clk not found\n");
+ }
+
+ sup->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+ if (IS_ERR(sup->rstc))
+ return dev_err_probe(&pdev->dev, PTR_ERR(sup->rstc), "rstc not found\n");
+
+ port = &sup->port;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ port->membase = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(port->membase))
+ return dev_err_probe(&pdev->dev, PTR_ERR(port->membase), "membase not found\n");
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ port->mapbase = res->start;
+ port->uartclk = clk_get_rate(sup->clk);
+ port->line = pdev->id;
+ port->irq = irq;
+ port->dev = &pdev->dev;
+ port->iotype = UPIO_MEM;
+ port->ops = &sunplus_uart_ops;
+ port->flags = UPF_BOOT_AUTOCONF;
+ port->fifosize = 128;
+
+ ret = reset_control_deassert(sup->rstc);
+ if (ret)
+ return ret;
+
+#ifdef CONFIG_SERIAL_SUNPLUS_CONSOLE
+ if (pdev->id == 0)
+ port->cons = &sunplus_uart_console;
+ sunplus_console_ports[sup->port.line] = sup;
+#endif
+
+ platform_set_drvdata(pdev, &sup->port);
+
+ ret = uart_add_one_port(&sunplus_uart_driver, &sup->port);
+#ifdef CONFIG_SERIAL_SUNPLUS_CONSOLE
+ if (ret)
+ sunplus_console_ports[sup->port.line] = NULL;
+#endif
+
+#ifdef CONFIG_PM
+ if (!uart_console(&sup->port)) {
+ pm_runtime_set_autosuspend_delay(&pdev->dev, UART_AUTOSUSPEND_TIMEOUT);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ }
+#endif
+ return ret;
+}
+
+static int sunplus_uart_remove(struct platform_device *pdev)
+{
+ struct sunplus_uart_port *sup = platform_get_drvdata(pdev);
+#ifdef CONFIG_PM
+ if (!uart_console(&sup->port)) {
+ pm_runtime_disable(&pdev->dev);
+ pm_runtime_set_suspended(&pdev->dev);
+ }
+#endif
+ uart_remove_one_port(&sunplus_uart_driver, &sup->port);
+
+ if (pdev->id < SUP_UART_NR) {
+ clk_disable_unprepare(sup->clk);
+ reset_control_assert(sup->rstc);
+ }
+ return 0;
+}
+
+static int sunplus_uart_suspend(struct device *dev)
+{
+ struct sunplus_uart_port *sup = dev_get_drvdata(dev);
+
+ if (!uart_console(&sup->port))
+ uart_suspend_port(&sunplus_uart_driver, &sup->port);
+
+ return 0;
+}
+
+static int sunplus_uart_resume(struct device *dev)
+{
+ struct sunplus_uart_port *sup = dev_get_drvdata(dev);
+
+ if (!uart_console(&sup->port))
+ uart_resume_port(&sunplus_uart_driver, &sup->port);
+
+ return 0;
+}
+
+static int __maybe_unused sunplus_runtime_suspend(struct device *dev)
+{
+ struct sunplus_uart_port *sup = dev_get_drvdata(dev);
+
+ if (!uart_console(&sup->port))
+ clk_disable_unprepare(sup->clk);
+
+ return 0;
+}
+
+static int __maybe_unused sunplus_runtime_resume(struct device *dev)
+{
+ struct sunplus_uart_port *sup = dev_get_drvdata(dev);
+
+ if (!uart_console(&sup->port))
+ clk_prepare_enable(sup->clk);
+
+ return 0;
+}
+
+static const struct dev_pm_ops sunplus_uart_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(sunplus_uart_suspend, sunplus_uart_resume)
+ SET_RUNTIME_PM_OPS(sunplus_runtime_suspend, sunplus_runtime_resume, NULL)
+};
+
+static const struct of_device_id sp_uart_of_match[] = {
+ { .compatible = "sunplus,sp7021-uart" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, sp_uart_of_match);
+
+static struct platform_driver sunplus_uart_platform_driver = {
+ .probe = sunplus_uart_probe,
+ .remove = sunplus_uart_remove,
+ .driver = {
+ .name = "sunplus_uart",
+ .of_match_table = of_match_ptr(sp_uart_of_match),
+ .pm = &sunplus_uart_pm_ops,
+ }
+};
+
+static int __init sunplus_uart_init(void)
+{
+ int ret;
+
+#ifdef CONFIG_SERIAL_SUNPLUS_CONSOLE
+ sunplus_uart_driver.cons = &sunplus_uart_console;
+#endif
+
+ ret = uart_register_driver(&sunplus_uart_driver);
+ if (ret)
+ return ret;
+
+ ret = platform_driver_register(&sunplus_uart_platform_driver);
+ if (ret)
+ uart_unregister_driver(&sunplus_uart_driver);
+
+ return ret;
+}
+
+static void __exit sunplus_uart_exit(void)
+{
+ platform_driver_unregister(&sunplus_uart_platform_driver);
+ uart_unregister_driver(&sunplus_uart_driver);
+}
+
+module_init(sunplus_uart_init);
+module_exit(sunplus_uart_exit);
+
+#ifdef CONFIG_SERIAL_EARLYCON
+static void sunplus_uart_putc(struct uart_port *port, int c)
+{
+ unsigned int val;
+
+ readl_poll_timeout_atomic(port->membase + SUP_UART_LSR, val,
+ (val & SUP_UART_LSR_TXE), 1, 10000);
+
+ writel(c, port->membase + SUP_UART_DATA);
+}
+
+static void sunplus_uart_early_write(struct console *con, const char *s, unsigned int n)
+{
+ struct earlycon_device *dev = con->data;
+
+ uart_console_write(&dev->port, s, n, sunplus_uart_putc);
+}
+
+static int __init
+sunplus_uart_early_setup(struct earlycon_device *dev, const char *opt)
+{
+ if (!(dev->port.membase || dev->port.iobase))
+ return -ENODEV;
+
+ dev->con->write = sunplus_uart_early_write;
+
+ return 0;
+}
+
+OF_EARLYCON_DECLARE(sunplus_uart, "sunplus,sp7021-uart", sunplus_uart_early_setup);
+#endif
+
+MODULE_DESCRIPTION("Sunplus UART driver");
+MODULE_AUTHOR("Hammer Hsieh <[email protected]>");
+MODULE_LICENSE("GPL v2");
--
2.7.4
On Thu, Dec 2, 2021 at 9:35 PM Hammer Hsieh <[email protected]> wrote:
>
> Add Sunplus SoC UART Driver
...
> +config SERIAL_SUNPLUS
> + tristate "Sunplus UART support"
> + depends on OF || COMPILE_TEST
> + select SERIAL_CORE
> + help
> + Select this option if you would like to use Sunplus serial port on
> + Sunplus SoC SP7021.
> + If you enable this option, Sunplus serial ports in the system will
> + be registered as ttySUPx.
What will be the module name in case of module build?
...
> +/*
> + * Sunplus SoC UART driver
> + *
> + * Author: Hammer Hsieh <[email protected]>
Authors:
> + * Tony Huang <[email protected]>
> + * Wells Lu <[email protected]>
And please indent names to be on the same column.
> + */
...
> +#define UART_AUTOSUSPEND_TIMEOUT 3000
Add units to the name.
...
> +static inline u32 sunplus_tx_buf_not_full(struct uart_port *port)
> +{
> + unsigned int lsr = readl(port->membase + SUP_UART_LSR);
> +
> + return ((lsr & SUP_UART_LSR_TX) ? SUP_UART_LSR_TX_NOT_FULL : 0);
Too many parentheses. Ditto for all similar cases.
> +}
...
> + do {
> + sp_uart_put_char(port, xmit->buf[xmit->tail]);
> + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
"% UART_XMIT_SIZE" is more accurate since it doesn't require a value
to be a power of 2. In case of power of 2 it will be properly
optimized by a compiler.
> + port->icount.tx++;
> +
> + if (uart_circ_empty(xmit))
> + break;
> + } while (sunplus_tx_buf_not_full(port));
...
> + spin_lock_irqsave(&port->lock, flags);
Why irqsave here?
...
> + if (readl(port->membase + SUP_UART_ISC) & SUP_UART_ISC_RX)
> + receive_chars(port);
> +
> + if (readl(port->membase + SUP_UART_ISC) & SUP_UART_ISC_TX)
> + transmit_chars(port);
Do you really need to perform two I/O against the very same register?
...
> +static int sunplus_startup(struct uart_port *port)
> +{
> + unsigned int isc;
> + int ret;
> +#ifdef CONFIG_PM
Why is this ifdeffery around the driver?
> + if (!uart_console(port)) {
> + ret = pm_runtime_get_sync(port->dev);
> + if (ret < 0)
> + goto out;
> + }
> +#endif
> + ret = request_irq(port->irq, sunplus_uart_irq, 0, "sunplus_uart", port);
> + if (ret)
> + return ret;
> +
> + spin_lock_irq(&port->lock);
> +
> + isc |= SUP_UART_ISC_RXM;
> + writel(isc, port->membase + SUP_UART_ISC);
> +
> + spin_unlock_irq(&port->lock);
> +#ifdef CONFIG_PM
> + if (!uart_console(port))
> + pm_runtime_put(port->dev);
Why doesn't it set autosuspend, i.o.w. Why is it different from an error case?
> + return 0;
> +out:
> + if (!uart_console(port)) {
> + pm_runtime_mark_last_busy(port->dev);
> + pm_runtime_put_autosuspend(port->dev);
> + }
> +#endif
> + return 0;
> +}
...
> +static void sunplus_set_termios(struct uart_port *port,
> + struct ktermios *termios,
> + struct ktermios *oldtermios)
> +{
> + u32 clk, ext, div, div_l, div_h, baud;
> + u32 lcr, val;
> + unsigned long flags;
> + clk = port->uartclk;
This can be done in the definition block above.
> + baud = uart_get_baud_rate(port, termios, oldtermios, 0, port->uartclk / 16);
> +
> + readl_poll_timeout_atomic(port->membase + SUP_UART_LSR, val,
> + (val & SUP_UART_LSR_TXE), 1, 10000);
No error check?
> + /*
> + * baud rate = uartclk / ((16 * div[15:0] + 1) + div_ext[3:0])
> + * get target baud rate and uartclk
> + * auto calculate div and div_ext
> + * div_h = (div[15:8] >> 8); div_l = (div_ext[3:0] << 12) + div[7:0]
There is no need to explain the code, please add excerpts from the
data sheet on how the divisors and baud rate are calculated, use
mathematical language, and not programming in the comment.
> + */
> + clk += baud >> 1;
> + div = clk / baud;
> + ext = div & 0x0F;
> + div = (div >> 4) - 1;
> + div_l = (div & 0xFF) | (ext << 12);
> + div_h = div >> 8;
...
> +static const char *sunplus_type(struct uart_port *port)
> +{
> + struct sunplus_uart_port *sup = to_sunplus_uart(port);
> +
> + return sup->port.type == PORT_SUNPLUS ? "sunplus_uart" : NULL;
> +}
What do we achieve with this? Who and how will see this information?
Under which circumstances the port type is not SUNPLUS?
...
> +static void sunplus_release_port(struct uart_port *port)
> +{
> +}
> +
> +static int sunplus_request_port(struct uart_port *port)
> +{
> + return 0;
> +}
Are these stubs mandatory?
...
> +static void sunplus_config_port(struct uart_port *port, int type)
> +{
> + if (type & UART_CONFIG_TYPE) {
> + port->type = PORT_SUNPLUS;
> + sunplus_request_port(port);
> + }
if (!(type & ...))
return;
?
> +}
...
> +static int sunplus_poll_init(struct uart_port *port)
> +{
> + return 0;
> +}
Why is this stub needed?
...
> +static inline void wait_for_xmitr(struct uart_port *port)
> +{
> + unsigned int val;
> +
> + readl_poll_timeout_atomic(port->membase + SUP_UART_LSR, val,
> + (val & SUP_UART_LSR_TX), 1, 10000);
Error handling?
Or explain why it's not needed.
> +}
...
> + sup->clk = devm_clk_get(&pdev->dev, NULL);
> + if (!IS_ERR(sup->clk)) {
Instead use devm_clk_get_optional().
> + ret = clk_prepare_enable(sup->clk);
> + if (ret)
> + return ret;
> + ret = devm_add_action_or_reset(&pdev->dev,
> + (void(*)(void *))clk_disable_unprepare,
> + sup->clk);
Look at the examples of how other drivers do this (that were submitted
more or less recently).
> + if (ret)
> + return ret;
> + } else {
> + if (PTR_ERR(sup->clk) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
Why?!
> + return dev_err_probe(&pdev->dev, PTR_ERR(sup->clk), "clk not found\n");
> + }
...
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + port->membase = devm_ioremap_resource(&pdev->dev, res);
We have an API that does both at once. Use it.
> + if (IS_ERR(port->membase))
> + return dev_err_probe(&pdev->dev, PTR_ERR(port->membase), "membase not found\n");
...
> + ret = reset_control_deassert(sup->rstc);
> + if (ret)
> + return ret;
From here no reset assertion on error? Why?
...
> +#ifdef CONFIG_SERIAL_SUNPLUS_CONSOLE
> + if (pdev->id == 0)
> + port->cons = &sunplus_uart_console;
> + sunplus_console_ports[sup->port.line] = sup;
> +#endif
Actually why don't you use register_console() ?
...
> +static struct platform_driver sunplus_uart_platform_driver = {
> + .probe = sunplus_uart_probe,
> + .remove = sunplus_uart_remove,
> + .driver = {
> + .name = "sunplus_uart",
> + .of_match_table = of_match_ptr(sp_uart_of_match),
How did you test this when OF=n and COMPILE_TEST=y?
Hint: Compiler will warn about unused variables (you need W=1).
> + .pm = &sunplus_uart_pm_ops,
> + }
> +};
...
> +static void __exit sunplus_uart_exit(void)
> +{
> + platform_driver_unregister(&sunplus_uart_platform_driver);
> + uart_unregister_driver(&sunplus_uart_driver);
> +}
> +
Dtop this blank line...
> +module_init(sunplus_uart_init);
> +module_exit(sunplus_uart_exit);
...and attach each of the calls to the implemented function.
...
> +static int __init
> +sunplus_uart_early_setup(struct earlycon_device *dev, const char *opt)
> +{
> + if (!(dev->port.membase || dev->port.iobase))
> + return -ENODEV;
> +
> + dev->con->write = sunplus_uart_early_write;
> +
> + return 0;
> +}
> +
No blank line.
> +OF_EARLYCON_DECLARE(sunplus_uart, "sunplus,sp7021-uart", sunplus_uart_early_setup);
--
With Best Regards,
Andy Shevchenko
Hi, Andy Shevchenko :
Thanks for your review.
Please see my response in below mail.
Regards,
Hammer Hsieh
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Friday, December 3, 2021 4:03 AM
> To: Hammer Hsieh <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Rob Herring
> <[email protected]>; open list:SERIAL DRIVERS
> <[email protected]>; devicetree <[email protected]>;
> Linux Kernel Mailing List <[email protected]>; Jiri Slaby
> <[email protected]>; Philipp Zabel <[email protected]>; Tony Huang
> 黃懷厚 <[email protected]>; Wells Lu 呂芳騰
> <[email protected]>; Hammer Hsieh 謝宏孟
> <[email protected]>
> Subject: Re: [PATCH v4 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver
>
> On Thu, Dec 2, 2021 at 9:35 PM Hammer Hsieh <[email protected]>
> wrote:
> >
> > Add Sunplus SoC UART Driver
>
> ...
>
> > +config SERIAL_SUNPLUS
> > + tristate "Sunplus UART support"
> > + depends on OF || COMPILE_TEST
> > + select SERIAL_CORE
> > + help
> > + Select this option if you would like to use Sunplus serial port on
> > + Sunplus SoC SP7021.
> > + If you enable this option, Sunplus serial ports in the system will
> > + be registered as ttySUPx.
>
> What will be the module name in case of module build?
>
will add info as below shows in Kconfig:
"This driver can also be built as a module. If so, the module will be called sunplus-uart."
> ...
>
> > +/*
> > + * Sunplus SoC UART driver
> > + *
> > + * Author: Hammer Hsieh <[email protected]>
>
> Authors:
>
> > + * Tony Huang <[email protected]>
> > + * Wells Lu <[email protected]>
>
> And please indent names to be on the same column.
>
I rewrite almost all uart driver.
The other authors ask me to remove their names on this driver.
Will modify it.
> > + */
>
> ...
>
> > +#define UART_AUTOSUSPEND_TIMEOUT 3000
>
> Add units to the name.
>
Will add it. /* units: ms */
> ...
>
> > +static inline u32 sunplus_tx_buf_not_full(struct uart_port *port) {
> > + unsigned int lsr = readl(port->membase + SUP_UART_LSR);
> > +
> > + return ((lsr & SUP_UART_LSR_TX) ? SUP_UART_LSR_TX_NOT_FULL :
> > + 0);
>
> Too many parentheses. Ditto for all similar cases.
>
> > +}
>
ok, I have found similar case. I will modify it.
> ...
>
> > + do {
> > + sp_uart_put_char(port, xmit->buf[xmit->tail]);
> > + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>
> "% UART_XMIT_SIZE" is more accurate since it doesn't require a value to be a
> power of 2. In case of power of 2 it will be properly optimized by a compiler.
>
Currently, I have found drivers/tty/serial/mps2-uart.c use it only.
ok, will modify it.
> > + port->icount.tx++;
> > +
> > + if (uart_circ_empty(xmit))
> > + break;
> > + } while (sunplus_tx_buf_not_full(port));
>
> ...
>
> > + spin_lock_irqsave(&port->lock, flags);
>
> Why irqsave here?
>
> ...
>
> > + if (readl(port->membase + SUP_UART_ISC) & SUP_UART_ISC_RX)
> > + receive_chars(port);
> > +
> > + if (readl(port->membase + SUP_UART_ISC) & SUP_UART_ISC_TX)
> > + transmit_chars(port);
>
> Do you really need to perform two I/O against the very same register?
>
I will rewrite as below, how do you think?
Static irqreturn_t sunplus_uart_irq(int irq, void *args)
{
Struct uart_port *port = (struct uart_port *)args;
unsigned int isc = readl(port->membase + SUP_UART_ISC);
if (isc & SUP_UART_ISC_RX)
receive_chars(port);
if (isc & SUP_UART_ISC_TX)
transmit_chars(port);
return IRQ_HANDLED;
}
> ...
>
> > +static int sunplus_startup(struct uart_port *port) {
> > + unsigned int isc;
> > + int ret;
>
> > +#ifdef CONFIG_PM
>
> Why is this ifdeffery around the driver?
>
> > + if (!uart_console(port)) {
> > + ret = pm_runtime_get_sync(port->dev);
> > + if (ret < 0)
> > + goto out;
> > + }
> > +#endif
> > + ret = request_irq(port->irq, sunplus_uart_irq, 0, "sunplus_uart",
> port);
> > + if (ret)
> > + return ret;
> > +
> > + spin_lock_irq(&port->lock);
> > +
> > + isc |= SUP_UART_ISC_RXM;
> > + writel(isc, port->membase + SUP_UART_ISC);
> > +
> > + spin_unlock_irq(&port->lock);
>
> > +#ifdef CONFIG_PM
> > + if (!uart_console(port))
> > + pm_runtime_put(port->dev);
>
> Why doesn't it set autosuspend, i.o.w. Why is it different from an error case?
>
Autosuspend already init at probe.
I remove #ifdef CONFIG_PM code in sunplus_startup() and test runtime function.
linux-serial-test -y 0x55 -z 0x30 -p /dev/ttySUP1 -b 115200
runtime_resume and runtime_suspend still work.
I will remove it.
> > + return 0;
> > +out:
> > + if (!uart_console(port)) {
> > + pm_runtime_mark_last_busy(port->dev);
> > + pm_runtime_put_autosuspend(port->dev);
> > + }
> > +#endif
> > + return 0;
> > +}
>
> ...
>
> > +static void sunplus_set_termios(struct uart_port *port,
> > + struct ktermios *termios,
> > + struct ktermios *oldtermios) {
> > + u32 clk, ext, div, div_l, div_h, baud;
> > + u32 lcr, val;
> > + unsigned long flags;
>
> > + clk = port->uartclk;
>
> This can be done in the definition block above.
>
I think you want the code like below, right ?
u32 ext, div, div_l, div_h, baud, lcr;
u32 clk = port->uartclk;
unsigned long flags;
> > + baud = uart_get_baud_rate(port, termios, oldtermios, 0,
> > + port->uartclk / 16);
> > +
> > + readl_poll_timeout_atomic(port->membase + SUP_UART_LSR, val,
> > + (val & SUP_UART_LSR_TXE), 1,
> 10000);
>
> No error check?
>
remove this code in set_termios( ).
I think it is not necessary to check tx empty here.
> > + /*
> > + * baud rate = uartclk / ((16 * div[15:0] + 1) + div_ext[3:0])
> > + * get target baud rate and uartclk
> > + * auto calculate div and div_ext
> > + * div_h = (div[15:8] >> 8); div_l = (div_ext[3:0] << 12) +
> > + div[7:0]
>
> There is no need to explain the code, please add excerpts from the data sheet
> on how the divisors and baud rate are calculated, use mathematical language,
> and not programming in the comment.
>
Ok, will modify it. Which one is better?
/* baud rate = uartclk / ((16 * divisor + 1) + divisor_ext) */
Or
/* uartclk
* baud rate = -------------------------------------------
* (16 * divisor + 1) + divisor_ext
*/
> > + */
> > + clk += baud >> 1;
> > + div = clk / baud;
> > + ext = div & 0x0F;
> > + div = (div >> 4) - 1;
> > + div_l = (div & 0xFF) | (ext << 12);
> > + div_h = div >> 8;
>
> ...
>
> > +static const char *sunplus_type(struct uart_port *port) {
> > + struct sunplus_uart_port *sup = to_sunplus_uart(port);
> > +
> > + return sup->port.type == PORT_SUNPLUS ? "sunplus_uart" : NULL;
> > +}
>
>
> What do we achieve with this? Who and how will see this information?
> Under which circumstances the port type is not SUNPLUS?
>
> ...
>
> > +static void sunplus_release_port(struct uart_port *port) { }
> > +
> > +static int sunplus_request_port(struct uart_port *port) {
> > + return 0;
> > +}
>
> Are these stubs mandatory?
>
> ...
>
> > +static void sunplus_config_port(struct uart_port *port, int type) {
>
> > + if (type & UART_CONFIG_TYPE) {
> > + port->type = PORT_SUNPLUS;
> > + sunplus_request_port(port);
> > + }
>
> if (!(type & ...))
> return;
>
> ?
>
About these functions
sunplus_type / sunplus_release_port / sunplus_request_port / sunplus_config_port
Almost all uart driver have these function, but actually I don't know when/how to use it.
I will study it.
If you have more information, please share it to me.
> > +}
>
> ...
>
> > +static int sunplus_poll_init(struct uart_port *port) {
> > + return 0;
> > +}
>
> Why is this stub needed?
>
Will remove it.
> ...
>
> > +static inline void wait_for_xmitr(struct uart_port *port) {
> > + unsigned int val;
> > +
> > + readl_poll_timeout_atomic(port->membase + SUP_UART_LSR, val,
> > + (val & SUP_UART_LSR_TX), 1,
> 10000);
>
> Error handling?
> Or explain why it's not needed.
>
> > +}
>
I refer to /drivers/tty/serial/owl-uart.c
Will add error handling for it as below , is that OK?
static inline void wait_for_xmitr(struct uart_port *port) {
unsigned int val;
int ret;
/*Wait for FIFO is full or timeout */
ret = readl_poll_timeout_atomic(port->membase + SUP_UART_LSR, val,
(val & SUP_UART_LSR_TX), 1, 10000);
If (ret == -ETIMEOUT) {
dev_err(port->dev, "Timeout waiting while UART TX FULL);
return;
}
}
> ...
>
> > + sup->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (!IS_ERR(sup->clk)) {
>
> Instead use devm_clk_get_optional().
>
> > + ret = clk_prepare_enable(sup->clk);
> > + if (ret)
> > + return ret;
>
> > + ret = devm_add_action_or_reset(&pdev->dev,
> > + (void(*)(void
> *))clk_disable_unprepare,
> > + sup->clk);
>
> Look at the examples of how other drivers do this (that were submitted more
> or less recently).
>
> > + if (ret)
> > + return ret;
> > + } else {
>
> > + if (PTR_ERR(sup->clk) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
>
> Why?!
>
> > + return dev_err_probe(&pdev->dev, PTR_ERR(sup->clk),
> "clk not found\n");
> > + }
>
> ...
In case of without deferred probe.
I refer to /drivers/tty/serial/meson_uart.c
And I will modify as below, is that ok?
sup->clk = devm_clk_get_optional(&pdev->dev, NULL);
if (IS_ERR(sup->clk))
return dev_err_probe(&pdev->dev, PTR_ERR(sup->clk), "clk not found\n");
ret = clk_prepare_enable(sup->clk);
if (ret)
return ret;
ret = devm_add_action_or_reset(&pdev->dev, (void(*)(void*))clk_disable_unprepare,
sup->clk);
if (ret)
return ret;
As your comment last patch v3 , you said "think of deferred".
So I refer to /drivers/tty/serial/sccnxp.c
Maybe I just need to do it without deferred probe.
>
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + port->membase = devm_ioremap_resource(&pdev->dev, res);
>
> We have an API that does both at once. Use it.
>
ok, will modify for it.
devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > + if (IS_ERR(port->membase))
> > + return dev_err_probe(&pdev->dev,
> > + PTR_ERR(port->membase), "membase not found\n");
>
> ...
>
> > + ret = reset_control_deassert(sup->rstc);
> > + if (ret)
> > + return ret;
>
> From here no reset assertion on error? Why?
>
I found I didn't add devm_add_action_or_reset( ) to run reset_control_assert( ) for it.
I will add it as bleow.
ret = devm_add_action_or_reset(&pdev->dev, (void(*)(void*))reset_control_assert,
sup->rstc);
if (ret)
return ret;
> ...
>
> > +#ifdef CONFIG_SERIAL_SUNPLUS_CONSOLE
> > + if (pdev->id == 0)
> > + port->cons = &sunplus_uart_console;
> > + sunplus_console_ports[sup->port.line] = sup; #endif
>
> Actually why don't you use register_console() ?
>
I will think how to do it.
> ...
>
> > +static struct platform_driver sunplus_uart_platform_driver = {
> > + .probe = sunplus_uart_probe,
> > + .remove = sunplus_uart_remove,
> > + .driver = {
> > + .name = "sunplus_uart",
>
> > + .of_match_table = of_match_ptr(sp_uart_of_match),
>
> How did you test this when OF=n and COMPILE_TEST=y?
> Hint: Compiler will warn about unused variables (you need W=1).
>
> > + .pm = &sunplus_uart_pm_ops,
> > + }
> > +};
>
I have found many patch drop of_match_ptr( ) cause warning unused variable.
Now I know what you means at last PATCH v3 comment.
I set OF=n and COMPILE_TEST=y, but other error come out first.
I didn't confirm it further.
Will remove of_match_ptr( ).
> ...
>
> > +static void __exit sunplus_uart_exit(void) {
> > + platform_driver_unregister(&sunplus_uart_platform_driver);
> > + uart_unregister_driver(&sunplus_uart_driver);
> > +}
>
> > +
>
> Dtop this blank line...
>
> > +module_init(sunplus_uart_init);
> > +module_exit(sunplus_uart_exit);
>
> ...and attach each of the calls to the implemented function.
>
Ok , will modify it.
> ...
>
> > +static int __init
> > +sunplus_uart_early_setup(struct earlycon_device *dev, const char
> > +*opt) {
> > + if (!(dev->port.membase || dev->port.iobase))
> > + return -ENODEV;
> > +
> > + dev->con->write = sunplus_uart_early_write;
> > +
> > + return 0;
> > +}
>
> > +
>
> No blank line.
>
Use scripts/checkpatch.pl --strict -f drivers/tty/serial/sunplus-uart.c
It will show "CHECK: Please use a blank line after function/struct/union/enum declarations".
That's why I confuse it and add a blank for it.
ok, will remove the blank.
> > +OF_EARLYCON_DECLARE(sunplus_uart, "sunplus,sp7021-uart",
> > +sunplus_uart_early_setup);
>
> --
> With Best Regards,
> Andy Shevchenko