Re: [PATCH 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver
From: Greg KH <gregkh@linuxfoundation.org>
Date: 2021-11-01 07:56:19
Also in:
linux-devicetree, lkml
On Mon, Nov 01, 2021 at 03:48:23PM +0800, hammer.hsieh wrote:
Add Sunplus SoC UART Driver Signed-off-by: hammer.hsieh <redacted>
I doubt you sign your name with a "." in it, right? Also this address does not match your From: address. Please fix this up when resending this.
quoted hunk ↗ jump to hunk
--- MAINTAINERS | 2 + drivers/tty/serial/Kconfig | 23 + drivers/tty/serial/Makefile | 1 + drivers/tty/serial/sunplus-uart.c | 1848 +++++++++++++++++++++++++++++++++++++ include/soc/sunplus/sp_uart.h | 158 ++++ 5 files changed, 2032 insertions(+) create mode 100644 drivers/tty/serial/sunplus-uart.c create mode 100644 include/soc/sunplus/sp_uart.hdiff --git a/MAINTAINERS b/MAINTAINERS index f863e97..0176026 100644 --- a/MAINTAINERS +++ b/MAINTAINERS@@ -17949,6 +17949,8 @@ SUNPLUS UART DRIVER M: Hammer Hsieh <hammer.hsieh@sunplus.com> S: Maintained F: Documentation/devicetree/bindings/serial/sunplus,uart.yaml +F: drivers/tty/serial/sunplus-uart.c +F: include/soc/sunplus/sp-uart.h SUPERH M: Yoshinori Sato <ysato@users.sourceforge.jp>diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig index 131a6a5..319f5db 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 + bool "Sunplus UART support" + depends on OF + 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 ttySx. + +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=ttySx". + endmenu config SERIAL_MCTRL_GPIOdiff --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.odiff --git a/drivers/tty/serial/sunplus-uart.c b/drivers/tty/serial/sunplus-uart.c new file mode 100644 index 0000000..d1524dc --- /dev/null +++ b/drivers/tty/serial/sunplus-uart.c@@ -0,0 +1,1848 @@ +// SPDX-License-Identifier: GPL-2.0-or-later
I have to ask,. but are you sure about this license that it needs to be "-or-later"?
+/* + * Sunplus SoC UART driver + * + * Author: Hammer Hsieh [off-list ref] + * Tony Huang [off-list ref] + * Wells Lu [off-list ref] + * + */ + +#include <linux/module.h> +#include <linux/init.h> +#include <linux/console.h> +#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <linux/tty.h> +#include <linux/tty_flip.h> +#include <linux/of_platform.h> +#include <asm/irq.h> +#if defined(CONFIG_MAGIC_SYSRQ) +#define SUPPORT_SYSRQ +#include <linux/sysrq.h> +#endif +#include <linux/serial_core.h> +#include <linux/clk.h> +#include <linux/reset.h> +#include <linux/io.h> +#include <linux/dma-mapping.h> +#include <soc/sunplus/sp_uart.h> + +#ifdef CONFIG_PM_RUNTIME_UART +#include <linux/pm_runtime.h> +#endif
Why the #ifdef in the .c file?
+ +#include <linux/gpio.h>
GPIO in a serial driver?
+#include <linux/of.h> +#include <linux/of_gpio.h> +#include <dt-bindings/clock/sp-sp7021.h> +#include <dt-bindings/pinctrl/sppctl-sp7021.h> +#include <linux/delay.h> +#include <linux/hrtimer.h>
Please put the linux/ includes above the dt-bindings ones.
+ +#define DEVICE_NAME "ttyS" +#define SP_UART_MAJOR TTY_MAJOR
Why is this driver allowed to be named the same as the other serial drivers? What happens when you plug in a 8250 card to this device?
+#define SP_UART_MINOR_START 64
Why here?
+ +#define NUM_UART 6 /* serial0, ... */ +#define NUM_UARTDMARX 2 /* serial10, ... */ +#define NUM_UARTDMATX 2 /* serial20, ... */
Why are these hard codeded?
+ +#define ID_BASE_DMARX 10 +#define ID_BASE_DMATX 20 + +#define IS_UARTDMARX_ID(X) \ + (((X) >= (ID_BASE_DMARX)) && ((X) < (ID_BASE_DMARX + NUM_UARTDMARX))) +#define IS_UARTDMATX_ID(X) \ + (((X) >= (ID_BASE_DMATX)) && ((X) < (ID_BASE_DMATX + NUM_UARTDMATX))) + +//#define TTYS_KDBG_INFO +#define TTYS_KDBG_ERR +//#define TTYS_GPIO
Why is this commented out? Why is this defined at all?
+
+#ifdef TTYS_KDBG_INFO
+#define DBG_INFO(fmt, args ...) pr_info("K_TTYS: " fmt, ## args)
+#else
+#define DBG_INFO(fmt, args ...)
+#endif
+#ifdef TTYS_KDBG_ERR
+#define DBG_ERR(fmt, args ...) pr_err("K_TTYS: " fmt, ## args)
+#else
+#define DBG_ERR(fmt, args ...)
+#endifNo, please use dev_info(), dev_err() and dev_dbg() only. Do not create your own logging macros for a single driver, use the ones the kernel gives you.
+ +#define SP_UART_CREAD_DISABLED (1 << 16) + +#define UARXDMA_BUF_SZ PAGE_SIZE +#define MAX_SZ_RXDMA_ISR (1 << 9) +#define UATXDMA_BUF_SZ PAGE_SIZE + +#define CLK_HIGH_UART 202500000 +#define UART_RATIO 232 + +#if defined(CONFIG_SP_MON)
What sets this? I've stopped reviewing here. thanks, greg k-h