[PATCH v10 0/2] Add Spreadtrum SoC bindings and serial driver support

[PATCH v10 0/2] Add Spreadtrum SoC bindings and serial driver support

[Chunyan Zhang]

Changes from v9:
	- Added SUPPORT_SYSRQ definition for sprd serial

Changes from v8:
	- Removed a few unuseful code lines.

Chunyan Zhang (2):
  Documentation: DT: Add bindings for Spreadtrum SoC Platform
  tty/serial: Add Spreadtrum sc9836-uart driver support

 Documentation/devicetree/bindings/arm/sprd.txt     |   11 +
 .../devicetree/bindings/serial/sprd-uart.txt       |    7 +
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 drivers/tty/serial/Kconfig                         |   18 +
 drivers/tty/serial/Makefile                        |    1 +
 drivers/tty/serial/sprd_serial.c                   |  793 ++++++++++++++++++++
 include/uapi/linux/serial_core.h                   |    3 +
 7 files changed, 834 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/sprd.txt
 create mode 100644 Documentation/devicetree/bindings/serial/sprd-uart.txt
 create mode 100644 drivers/tty/serial/sprd_serial.c

-- 
1.7.9.5

[PATCH v10 1/2] Documentation: DT: Add bindings for Spreadtrum SoC Platform

[Chunyan Zhang]

Adds Spreadtrum's prefix "sprd" to vendor-prefixes file.
Adds the devicetree binding documentations for Spreadtrum's sc9836-uart
and SC9836 SoC based on the Sharkl64 Platform which is a 64-bit SoC
Platform of Spreadtrum.

Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
---
 Documentation/devicetree/bindings/arm/sprd.txt     |   11 +++++++++++
 .../devicetree/bindings/serial/sprd-uart.txt       |    7 +++++++
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 3 files changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/sprd.txt
 create mode 100644 Documentation/devicetree/bindings/serial/sprd-uart.txt

diff --git a/Documentation/devicetree/bindings/arm/sprd.txt b/Documentation/devicetree/bindings/arm/sprd.txt
new file mode 100644
index 0000000..31a629d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/sprd.txt
@@ -0,0 +1,11 @@
+Spreadtrum SoC Platforms Device Tree Bindings
+----------------------------------------------------
+
+Sharkl64 is a Spreadtrum's SoC Platform which is based
+on ARM 64-bit processor.
+
+SC9836 openphone board with SC9836 SoC based on the
+Sharkl64 Platform shall have the following properties.
+
+Required root node properties:
+        - compatible = "sprd,sc9836-openphone", "sprd,sc9836";
diff --git a/Documentation/devicetree/bindings/serial/sprd-uart.txt b/Documentation/devicetree/bindings/serial/sprd-uart.txt
new file mode 100644
index 0000000..2aff0f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/sprd-uart.txt
@@ -0,0 +1,7 @@
+* Spreadtrum serial UART
+
+Required properties:
+- compatible: must be "sprd,sc9836-uart"
+- reg: offset and length of the register set for the device
+- interrupts: exactly one interrupt specifier
+- clocks: phandles to input clocks.
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index b1df0ad..0a8384f 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -153,6 +153,7 @@ snps	Synopsys, Inc.
 solidrun	SolidRun
 sony	Sony Corporation
 spansion	Spansion Inc.
+sprd	Spreadtrum Communications Inc.
 st	STMicroelectronics
 ste	ST-Ericsson
 stericsson	ST-Ericsson
-- 
1.7.9.5

[PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support

[Chunyan Zhang]

Add a full sc9836-uart driver for SC9836 SoC which is based on the
spreadtrum sharkl64 platform.
This driver also support earlycon.

Originally-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/Kconfig       |   18 +
 drivers/tty/serial/Makefile      |    1 +
 drivers/tty/serial/sprd_serial.c |  793 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |    3 +
 4 files changed, 815 insertions(+)
 create mode 100644 drivers/tty/serial/sprd_serial.c

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index c79b43c..13211f7 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1577,6 +1577,24 @@ config SERIAL_MEN_Z135
 	  This driver can also be build as a module. If so, the module will be called
 	  men_z135_uart.ko
 
+config SERIAL_SPRD
+	tristate "Support for Spreadtrum serial"
+	depends on ARCH_SPRD
+	select SERIAL_CORE
+	help
+	  This enables the driver for the Spreadtrum's serial.
+
+config SERIAL_SPRD_CONSOLE
+	bool "Spreadtrum UART console support"
+	depends on SERIAL_SPRD=y
+	select SERIAL_CORE_CONSOLE
+	select SERIAL_EARLYCON
+	help
+	  Support for early debug console using Spreadtrum's serial. This enables
+	  the console before standard serial driver is probed. This is enabled
+	  with "earlycon" on the kernel command line. The console is
+	  enabled when early_param is processed.
+
 endmenu
 
 config SERIAL_MCTRL_GPIO
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index 9a548ac..4801aca 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -93,6 +93,7 @@ obj-$(CONFIG_SERIAL_ARC)	+= arc_uart.o
 obj-$(CONFIG_SERIAL_RP2)	+= rp2.o
 obj-$(CONFIG_SERIAL_FSL_LPUART)	+= fsl_lpuart.o
 obj-$(CONFIG_SERIAL_MEN_Z135)	+= men_z135_uart.o
+obj-$(CONFIG_SERIAL_SPRD) += sprd_serial.o
 
 # GPIOLIB helpers for modem control lines
 obj-$(CONFIG_SERIAL_MCTRL_GPIO)	+= serial_mctrl_gpio.o
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
new file mode 100644
index 0000000..594b633
--- /dev/null
+++ b/drivers/tty/serial/sprd_serial.c
@@ -0,0 +1,793 @@
+/*
+ * Copyright (C) 2012-2015 Spreadtrum Communications Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#if defined(CONFIG_SERIAL_SPRD_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
+#define SUPPORT_SYSRQ
+#endif
+
+#include <linux/clk.h>
+#include <linux/console.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/serial_core.h>
+#include <linux/serial.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+
+/* device name */
+#define UART_NR_MAX		8
+#define SPRD_TTY_NAME		"ttyS"
+#define SPRD_FIFO_SIZE		128
+#define SPRD_DEF_RATE		26000000
+#define SPRD_BAUD_IO_LIMIT	3000000
+#define SPRD_TIMEOUT		256
+
+/* the offset of serial registers and BITs for them */
+/* data registers */
+#define SPRD_TXD		0x0000
+#define SPRD_RXD		0x0004
+
+/* line status register and its BITs  */
+#define SPRD_LSR		0x0008
+#define SPRD_LSR_OE		BIT(4)
+#define SPRD_LSR_FE		BIT(3)
+#define SPRD_LSR_PE		BIT(2)
+#define SPRD_LSR_BI		BIT(7)
+#define SPRD_LSR_TX_OVER	BIT(15)
+
+/* data number in TX and RX fifo */
+#define SPRD_STS1		0x000C
+
+/* interrupt enable register and its BITs */
+#define SPRD_IEN		0x0010
+#define SPRD_IEN_RX_FULL	BIT(0)
+#define SPRD_IEN_TX_EMPTY	BIT(1)
+#define SPRD_IEN_BREAK_DETECT	BIT(7)
+#define SPRD_IEN_TIMEOUT	BIT(13)
+
+/* interrupt clear register */
+#define SPRD_ICLR		0x0014
+
+/* line control register */
+#define SPRD_LCR		0x0018
+#define SPRD_LCR_STOP_1BIT	0x10
+#define SPRD_LCR_STOP_2BIT	0x30
+#define SPRD_LCR_DATA_LEN	(BIT(2) | BIT(3))
+#define SPRD_LCR_DATA_LEN5	0x0
+#define SPRD_LCR_DATA_LEN6	0x4
+#define SPRD_LCR_DATA_LEN7	0x8
+#define SPRD_LCR_DATA_LEN8	0xc
+#define SPRD_LCR_PARITY	(BIT(0) | BIT(1))
+#define SPRD_LCR_PARITY_EN	0x2
+#define SPRD_LCR_EVEN_PAR	0x0
+#define SPRD_LCR_ODD_PAR	0x1
+
+/* control register 1 */
+#define SPRD_CTL1			0x001C
+#define RX_HW_FLOW_CTL_THLD	BIT(6)
+#define RX_HW_FLOW_CTL_EN	BIT(7)
+#define TX_HW_FLOW_CTL_EN	BIT(8)
+#define RX_TOUT_THLD_DEF	0x3E00
+#define RX_HFC_THLD_DEF	0x40
+
+/* fifo threshold register */
+#define SPRD_CTL2		0x0020
+#define THLD_TX_EMPTY	0x40
+#define THLD_RX_FULL	0x40
+
+/* config baud rate register */
+#define SPRD_CLKD0		0x0024
+#define SPRD_CLKD1		0x0028
+
+/* interrupt mask status register */
+#define SPRD_IMSR			0x002C
+#define SPRD_IMSR_RX_FIFO_FULL		BIT(0)
+#define SPRD_IMSR_TX_FIFO_EMPTY	BIT(1)
+#define SPRD_IMSR_BREAK_DETECT		BIT(7)
+#define SPRD_IMSR_TIMEOUT		BIT(13)
+
+struct reg_backup {
+	u32 ien;
+	u32 ctrl0;
+	u32 ctrl1;
+	u32 ctrl2;
+	u32 clkd0;
+	u32 clkd1;
+	u32 dspwait;
+};
+
+struct sprd_uart_port {
+	struct uart_port port;
+	struct reg_backup reg_bak;
+	char name[16];
+};
+
+static struct sprd_uart_port *sprd_port[UART_NR_MAX];
+static int sprd_ports_num;
+
+static inline unsigned int serial_in(struct uart_port *port, int offset)
+{
+	return readl_relaxed(port->membase + offset);
+}
+
+static inline void serial_out(struct uart_port *port, int offset, int value)
+{
+	writel_relaxed(value, port->membase + offset);
+}
+
+static unsigned int sprd_tx_empty(struct uart_port *port)
+{
+	if (serial_in(port, SPRD_STS1) & 0xff00)
+		return 0;
+	else
+		return TIOCSER_TEMT;
+}
+
+static unsigned int sprd_get_mctrl(struct uart_port *port)
+{
+	return TIOCM_DSR | TIOCM_CTS;
+}
+
+static void sprd_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+	/* nothing to do */
+}
+
+static void sprd_stop_tx(struct uart_port *port)
+{
+	unsigned int ien, iclr;
+
+	iclr = serial_in(port, SPRD_ICLR);
+	ien = serial_in(port, SPRD_IEN);
+
+	iclr |= SPRD_IEN_TX_EMPTY;
+	ien &= ~SPRD_IEN_TX_EMPTY;
+
+	serial_out(port, SPRD_ICLR, iclr);
+	serial_out(port, SPRD_IEN, ien);
+}
+
+static void sprd_start_tx(struct uart_port *port)
+{
+	unsigned int ien;
+
+	ien = serial_in(port, SPRD_IEN);
+	if (!(ien & SPRD_IEN_TX_EMPTY)) {
+		ien |= SPRD_IEN_TX_EMPTY;
+		serial_out(port, SPRD_IEN, ien);
+	}
+}
+
+static void sprd_stop_rx(struct uart_port *port)
+{
+	unsigned int ien, iclr;
+
+	iclr = serial_in(port, SPRD_ICLR);
+	ien = serial_in(port, SPRD_IEN);
+
+	ien &= ~(SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT);
+	iclr |= SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT;
+
+	serial_out(port, SPRD_IEN, ien);
+	serial_out(port, SPRD_ICLR, iclr);
+}
+
+/* The Sprd serial does not support this function. */
+static void sprd_break_ctl(struct uart_port *port, int break_state)
+{
+	/* nothing to do */
+}
+
+static int handle_lsr_errors(struct uart_port *port,
+			     unsigned int *flag,
+			     unsigned int *lsr)
+{
+	int ret = 0;
+
+	/* statistics */
+	if (*lsr & SPRD_LSR_BI) {
+		*lsr &= ~(SPRD_LSR_FE | SPRD_LSR_PE);
+		port->icount.brk++;
+		ret = uart_handle_break(port);
+		if (ret)
+			return ret;
+	} else if (*lsr & SPRD_LSR_PE)
+		port->icount.parity++;
+	else if (*lsr & SPRD_LSR_FE)
+		port->icount.frame++;
+	if (*lsr & SPRD_LSR_OE)
+		port->icount.overrun++;
+
+	/* mask off conditions which should be ignored */
+	*lsr &= port->read_status_mask;
+	if (*lsr & SPRD_LSR_BI)
+		*flag = TTY_BREAK;
+	else if (*lsr & SPRD_LSR_PE)
+		*flag = TTY_PARITY;
+	else if (*lsr & SPRD_LSR_FE)
+		*flag = TTY_FRAME;
+
+	return ret;
+}
+
+static inline void sprd_rx(struct uart_port *port)
+{
+	struct tty_port *tty = &port->state->port;
+	unsigned int ch, flag, lsr, max_count = SPRD_TIMEOUT;
+
+	while ((serial_in(port, SPRD_STS1) & 0x00ff) && max_count--) {
+		lsr = serial_in(port, SPRD_LSR);
+		ch = serial_in(port, SPRD_RXD);
+		flag = TTY_NORMAL;
+		port->icount.rx++;
+
+		if (lsr & (SPRD_LSR_BI | SPRD_LSR_PE |
+			SPRD_LSR_FE | SPRD_LSR_OE))
+			if (handle_lsr_errors(port, &lsr, &flag))
+				continue;
+		if (uart_handle_sysrq_char(port, ch))
+			continue;
+
+		uart_insert_char(port, lsr, SPRD_LSR_OE, ch, flag);
+	}
+
+	tty_flip_buffer_push(tty);
+}
+
+static inline void sprd_tx(struct uart_port *port)
+{
+	struct circ_buf *xmit = &port->state->xmit;
+	int count;
+
+	if (port->x_char) {
+		serial_out(port, SPRD_TXD, port->x_char);
+		port->icount.tx++;
+		port->x_char = 0;
+		return;
+	}
+
+	if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
+		sprd_stop_tx(port);
+		return;
+	}
+
+	count = THLD_TX_EMPTY;
+	do {
+		serial_out(port, SPRD_TXD, xmit->buf[xmit->tail]);
+		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
+		port->icount.tx++;
+		if (uart_circ_empty(xmit))
+			break;
+	} while (--count > 0);
+
+	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+		uart_write_wakeup(port);
+
+	if (uart_circ_empty(xmit))
+		sprd_stop_tx(port);
+}
+
+/* this handles the interrupt from one port */
+static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
+{
+	struct uart_port *port = dev_id;
+	unsigned int ims;
+
+	spin_lock(&port->lock);
+
+	ims = serial_in(port, SPRD_IMSR);
+
+	if (!ims)
+		return IRQ_NONE;
+
+	serial_out(port, SPRD_ICLR, ~0);
+
+	if (ims & (SPRD_IMSR_RX_FIFO_FULL |
+		SPRD_IMSR_BREAK_DETECT | SPRD_IMSR_TIMEOUT))
+		sprd_rx(port);
+
+	if (ims & SPRD_IMSR_TX_FIFO_EMPTY)
+		sprd_tx(port);
+
+	spin_unlock(&port->lock);
+
+	return IRQ_HANDLED;
+}
+
+static int sprd_startup(struct uart_port *port)
+{
+	int ret = 0;
+	unsigned int ien, fc;
+	unsigned int timeout;
+	struct sprd_uart_port *sp;
+	unsigned long flags;
+
+	serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY << 8) | THLD_RX_FULL));
+
+	/* clear rx fifo */
+	timeout = SPRD_TIMEOUT;
+	while (timeout-- && serial_in(port, SPRD_STS1) & 0x00ff)
+		serial_in(port, SPRD_RXD);
+
+	/* clear tx fifo */
+	timeout = SPRD_TIMEOUT;
+	while (timeout-- && serial_in(port, SPRD_STS1) & 0xff00)
+		cpu_relax();
+
+	/* clear interrupt */
+	serial_out(port, SPRD_IEN, 0);
+	serial_out(port, SPRD_ICLR, ~0);
+
+	/* allocate irq */
+	sp = container_of(port, struct sprd_uart_port, port);
+	snprintf(sp->name, sizeof(sp->name), "sprd_serial%d", port->line);
+	ret = devm_request_irq(port->dev, port->irq, sprd_handle_irq,
+				IRQF_SHARED, sp->name, port);
+	if (ret) {
+		dev_err(port->dev, "fail to request serial irq %d, ret=%d\n",
+			port->irq, ret);
+		return ret;
+	}
+	fc = serial_in(port, SPRD_CTL1);
+	fc |= RX_TOUT_THLD_DEF | RX_HFC_THLD_DEF;
+	serial_out(port, SPRD_CTL1, fc);
+
+	/* enable interrupt */
+	spin_lock_irqsave(&port->lock, flags);
+	ien = serial_in(port, SPRD_IEN);
+	ien |= SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT | SPRD_IEN_TIMEOUT;
+	serial_out(port, SPRD_IEN, ien);
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	return 0;
+}
+
+static void sprd_shutdown(struct uart_port *port)
+{
+	serial_out(port, SPRD_IEN, 0);
+	serial_out(port, SPRD_ICLR, ~0);
+	devm_free_irq(port->dev, port->irq, port);
+}
+
+static void sprd_set_termios(struct uart_port *port,
+				    struct ktermios *termios,
+				    struct ktermios *old)
+{
+	unsigned int baud, quot;
+	unsigned int lcr = 0, fc;
+	unsigned long flags;
+
+	/* ask the core to calculate the divisor for us */
+	baud = uart_get_baud_rate(port, termios, old, 0, SPRD_BAUD_IO_LIMIT);
+
+	quot = (unsigned int)((port->uartclk + baud / 2) / baud);
+
+	/* set data length */
+	switch (termios->c_cflag & CSIZE) {
+	case CS5:
+		lcr |= SPRD_LCR_DATA_LEN5;
+		break;
+	case CS6:
+		lcr |= SPRD_LCR_DATA_LEN6;
+		break;
+	case CS7:
+		lcr |= SPRD_LCR_DATA_LEN7;
+		break;
+	case CS8:
+	default:
+		lcr |= SPRD_LCR_DATA_LEN8;
+		break;
+	}
+
+	/* calculate stop bits */
+	lcr &= ~(SPRD_LCR_STOP_1BIT | SPRD_LCR_STOP_2BIT);
+	if (termios->c_cflag & CSTOPB)
+		lcr |= SPRD_LCR_STOP_2BIT;
+	else
+		lcr |= SPRD_LCR_STOP_1BIT;
+
+	/* calculate parity */
+	lcr &= ~SPRD_LCR_PARITY;
+	termios->c_cflag &= ~CMSPAR;	/* no support mark/space */
+	if (termios->c_cflag & PARENB) {
+		lcr |= SPRD_LCR_PARITY_EN;
+		if (termios->c_cflag & PARODD)
+			lcr |= SPRD_LCR_ODD_PAR;
+		else
+			lcr |= SPRD_LCR_EVEN_PAR;
+	}
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	/* update the per-port timeout */
+	uart_update_timeout(port, termios->c_cflag, baud);
+
+	port->read_status_mask = SPRD_LSR_OE;
+	if (termios->c_iflag & INPCK)
+		port->read_status_mask |= SPRD_LSR_FE | SPRD_LSR_PE;
+	if (termios->c_iflag & (IGNBRK | BRKINT | PARMRK))
+		port->read_status_mask |= SPRD_LSR_BI;
+
+	/* characters to ignore */
+	port->ignore_status_mask = 0;
+	if (termios->c_iflag & IGNPAR)
+		port->ignore_status_mask |= SPRD_LSR_PE | SPRD_LSR_FE;
+	if (termios->c_iflag & IGNBRK) {
+		port->ignore_status_mask |= SPRD_LSR_BI;
+		/*
+		 * If we're ignoring parity and break indicators,
+		 * ignore overruns too (for real raw support).
+		 */
+		if (termios->c_iflag & IGNPAR)
+			port->ignore_status_mask |= SPRD_LSR_OE;
+	}
+
+	/* flow control */
+	fc = serial_in(port, SPRD_CTL1);
+	fc &= ~(RX_HW_FLOW_CTL_THLD | RX_HW_FLOW_CTL_EN | TX_HW_FLOW_CTL_EN);
+	if (termios->c_cflag & CRTSCTS) {
+		fc |= RX_HW_FLOW_CTL_THLD;
+		fc |= RX_HW_FLOW_CTL_EN;
+		fc |= TX_HW_FLOW_CTL_EN;
+	}
+
+	/* clock divider bit0~bit15 */
+	serial_out(port, SPRD_CLKD0, quot & 0xffff);
+
+	/* clock divider bit16~bit20 */
+	serial_out(port, SPRD_CLKD1, (quot & 0x1f0000) >> 16);
+	serial_out(port, SPRD_LCR, lcr);
+	fc |= RX_TOUT_THLD_DEF | RX_HFC_THLD_DEF;
+	serial_out(port, SPRD_CTL1, fc);
+
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	/* Don't rewrite B0 */
+	if (tty_termios_baud_rate(termios))
+		tty_termios_encode_baud_rate(termios, baud, baud);
+}
+
+static const char *sprd_type(struct uart_port *port)
+{
+	return "SPX";
+}
+
+static void sprd_release_port(struct uart_port *port)
+{
+	/* nothing to do */
+}
+
+static int sprd_request_port(struct uart_port *port)
+{
+	return 0;
+}
+
+static void sprd_config_port(struct uart_port *port, int flags)
+{
+	if (flags & UART_CONFIG_TYPE)
+		port->type = PORT_SPRD;
+}
+
+static int sprd_verify_port(struct uart_port *port,
+				   struct serial_struct *ser)
+{
+	if (ser->type != PORT_SPRD)
+		return -EINVAL;
+	if (port->irq != ser->irq)
+		return -EINVAL;
+	return 0;
+}
+
+static struct uart_ops serial_sprd_ops = {
+	.tx_empty = sprd_tx_empty,
+	.get_mctrl = sprd_get_mctrl,
+	.set_mctrl = sprd_set_mctrl,
+	.stop_tx = sprd_stop_tx,
+	.start_tx = sprd_start_tx,
+	.stop_rx = sprd_stop_rx,
+	.break_ctl = sprd_break_ctl,
+	.startup = sprd_startup,
+	.shutdown = sprd_shutdown,
+	.set_termios = sprd_set_termios,
+	.type = sprd_type,
+	.release_port = sprd_release_port,
+	.request_port = sprd_request_port,
+	.config_port = sprd_config_port,
+	.verify_port = sprd_verify_port,
+};
+
+#ifdef CONFIG_SERIAL_SPRD_CONSOLE
+static inline void wait_for_xmitr(struct uart_port *port)
+{
+	unsigned int status, tmout = 10000;
+
+	/* wait up to 10ms for the character(s) to be sent */
+	do {
+		status = serial_in(port, SPRD_STS1);
+		if (--tmout == 0)
+			break;
+		udelay(1);
+	} while (status & 0xff00);
+}
+
+static void sprd_console_putchar(struct uart_port *port, int ch)
+{
+	wait_for_xmitr(port);
+	serial_out(port, SPRD_TXD, ch);
+}
+
+static void sprd_console_write(struct console *co, const char *s,
+				      unsigned int count)
+{
+	struct uart_port *port = &sprd_port[co->index]->port;
+	int locked = 1;
+	unsigned long flags;
+
+	if (port->sysrq)
+		locked = 0;
+	else if (oops_in_progress)
+		locked = spin_trylock_irqsave(&port->lock, flags);
+	else
+		spin_lock_irqsave(&port->lock, flags);
+
+	uart_console_write(port, s, count, sprd_console_putchar);
+
+	/* wait for transmitter to become empty */
+	wait_for_xmitr(port);
+
+	if (locked)
+		spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static int __init sprd_console_setup(struct console *co, char *options)
+{
+	struct uart_port *port;
+	int baud = 115200;
+	int bits = 8;
+	int parity = 'n';
+	int flow = 'n';
+
+	if (co->index >= UART_NR_MAX || co->index < 0)
+		co->index = 0;
+
+	port = &sprd_port[co->index]->port;
+	if (port == NULL) {
+		pr_info("serial port %d not yet initialized\n", co->index);
+		return -ENODEV;
+	}
+	if (options)
+		uart_parse_options(options, &baud, &parity, &bits, &flow);
+
+	return uart_set_options(port, co, baud, parity, bits, flow);
+}
+
+static struct uart_driver sprd_uart_driver;
+static struct console sprd_console = {
+	.name = SPRD_TTY_NAME,
+	.write = sprd_console_write,
+	.device = uart_console_device,
+	.setup = sprd_console_setup,
+	.flags = CON_PRINTBUFFER,
+	.index = -1,
+	.data = &sprd_uart_driver,
+};
+
+#define SPRD_CONSOLE	(&sprd_console)
+
+/* Support for earlycon */
+static void sprd_putc(struct uart_port *port, int c)
+{
+	unsigned int timeout = SPRD_TIMEOUT;
+
+	while (timeout-- &&
+		   !(readl(port->membase + SPRD_LSR) & SPRD_LSR_TX_OVER))
+		cpu_relax();
+
+	writeb(c, port->membase + SPRD_TXD);
+}
+
+static void sprd_early_write(struct console *con, const char *s,
+				    unsigned n)
+{
+	struct earlycon_device *dev = con->data;
+
+	uart_console_write(&dev->port, s, n, sprd_putc);
+}
+
+static int __init sprd_early_console_setup(
+				struct earlycon_device *device,
+				const char *opt)
+{
+	if (!device->port.membase)
+		return -ENODEV;
+
+	device->con->write = sprd_early_write;
+	return 0;
+}
+
+EARLYCON_DECLARE(sprd_serial, sprd_early_console_setup);
+OF_EARLYCON_DECLARE(sprd_serial, "sprd,sc9836-uart",
+		    sprd_early_console_setup);
+
+#else /* !CONFIG_SERIAL_SPRD_CONSOLE */
+#define SPRD_CONSOLE		NULL
+#endif
+
+static struct uart_driver sprd_uart_driver = {
+	.owner = THIS_MODULE,
+	.driver_name = "sprd_serial",
+	.dev_name = SPRD_TTY_NAME,
+	.major = 0,
+	.minor = 0,
+	.nr = UART_NR_MAX,
+	.cons = SPRD_CONSOLE,
+};
+
+static int sprd_probe_dt_alias(int index, struct device *dev)
+{
+	struct device_node *np;
+	int ret = index;
+
+	if (!IS_ENABLED(CONFIG_OF))
+		return ret;
+
+	np = dev->of_node;
+	if (!np)
+		return ret;
+
+	ret = of_alias_get_id(np, "serial");
+	if (IS_ERR_VALUE(ret))
+		ret = index;
+	else if (ret >= ARRAY_SIZE(sprd_port) || sprd_port[ret] != NULL) {
+		dev_warn(dev, "requested serial port %d not available.\n", ret);
+		ret = index;
+	}
+
+	return ret;
+}
+
+static int sprd_remove(struct platform_device *dev)
+{
+	struct sprd_uart_port *sup = platform_get_drvdata(dev);
+
+	if (sup) {
+		uart_remove_one_port(&sprd_uart_driver, &sup->port);
+		sprd_port[sup->port.line] = NULL;
+		sprd_ports_num--;
+	}
+
+	if (!sprd_ports_num)
+		uart_unregister_driver(&sprd_uart_driver);
+
+	return 0;
+}
+
+static int sprd_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct uart_port *up;
+	struct clk *clk;
+	int irq;
+	int index;
+	int ret;
+
+	for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
+		if (sprd_port[index] == NULL)
+			break;
+
+	if (index == ARRAY_SIZE(sprd_port))
+		return -EBUSY;
+
+	index = sprd_probe_dt_alias(index, &pdev->dev);
+
+	sprd_port[index] = devm_kzalloc(&pdev->dev,
+		sizeof(*sprd_port[index]), GFP_KERNEL);
+	if (!sprd_port[index])
+		return -ENOMEM;
+
+	up = &sprd_port[index]->port;
+	up->dev = &pdev->dev;
+	up->line = index;
+	up->type = PORT_SPRD;
+	up->iotype = SERIAL_IO_PORT;
+	up->uartclk = SPRD_DEF_RATE;
+	up->fifosize = SPRD_FIFO_SIZE;
+	up->ops = &serial_sprd_ops;
+	up->flags = UPF_BOOT_AUTOCONF;
+
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (!IS_ERR(clk))
+		up->uartclk = clk_get_rate(clk);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "not provide mem resource\n");
+		return -ENODEV;
+	}
+	up->mapbase = res->start;
+	up->membase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(up->membase))
+		return PTR_ERR(up->membase);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "not provide irq resource\n");
+		return -ENODEV;
+	}
+	up->irq = irq;
+
+	if (!sprd_ports_num) {
+		ret = uart_register_driver(&sprd_uart_driver);
+		if (ret < 0) {
+			pr_err("Failed to register SPRD-UART driver\n");
+			return ret;
+		}
+	}
+	sprd_ports_num++;
+
+	ret = uart_add_one_port(&sprd_uart_driver, up);
+	if (ret) {
+		sprd_port[index] = NULL;
+		sprd_remove(pdev);
+	}
+
+	platform_set_drvdata(pdev, up);
+
+	return ret;
+}
+
+static int sprd_suspend(struct device *dev)
+{
+	struct sprd_uart_port *sup = dev_get_drvdata(dev);
+
+	uart_suspend_port(&sprd_uart_driver, &sup->port);
+
+	return 0;
+}
+
+static int sprd_resume(struct device *dev)
+{
+	struct sprd_uart_port *sup = dev_get_drvdata(dev);
+
+	uart_resume_port(&sprd_uart_driver, &sup->port);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(sprd_pm_ops, sprd_suspend, sprd_resume);
+
+static const struct of_device_id serial_ids[] = {
+	{.compatible = "sprd,sc9836-uart",},
+	{}
+};
+
+static struct platform_driver sprd_platform_driver = {
+	.probe		= sprd_probe,
+	.remove		= sprd_remove,
+	.driver		= {
+		.name	= "sprd_serial",
+		.of_match_table = of_match_ptr(serial_ids),
+		.pm	= &sprd_pm_ops,
+	},
+};
+
+module_platform_driver(sprd_platform_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Spreadtrum SoC serial driver series");
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index c172180..7e6eb39 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -248,4 +248,7 @@
 /* MESON */
 #define PORT_MESON	109
 
+/* SPRD SERIAL  */
+#define PORT_SPRD	110
+
 #endif /* _UAPILINUX_SERIAL_CORE_H */
-- 
1.7.9.5

[PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support

[Varka Bhadram]

Hi,

On Wednesday 28 January 2015 04:38 PM, Chunyan Zhang wrote:
> Add a full sc9836-uart driver for SC9836 SoC which is based on the
> spreadtrum sharkl64 platform.
> This driver also support earlycon.
>
> Originally-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>   drivers/tty/serial/Kconfig       |   18 +
>   drivers/tty/serial/Makefile      |    1 +
>   drivers/tty/serial/sprd_serial.c |  793 ++++++++++++++++++++++++++++++++++++++
>   include/uapi/linux/serial_core.h |    3 +
>   4 files changed, 815 insertions(+)
>   create mode 100644 drivers/tty/serial/sprd_serial.c
>
(...)

> +static int sprd_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct uart_port *up;
> +	struct clk *clk;
> +	int irq;
> +	int index;
> +	int ret;
> +
> +	for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
> +		if (sprd_port[index] == NULL)
> +			break;
> +
> +	if (index == ARRAY_SIZE(sprd_port))
> +		return -EBUSY;
> +
> +	index = sprd_probe_dt_alias(index, &pdev->dev);
> +
> +	sprd_port[index] = devm_kzalloc(&pdev->dev,
> +		sizeof(*sprd_port[index]), GFP_KERNEL);
> +	if (!sprd_port[index])
> +		return -ENOMEM;
> +
> +	up = &sprd_port[index]->port;
> +	up->dev = &pdev->dev;
> +	up->line = index;
> +	up->type = PORT_SPRD;
> +	up->iotype = SERIAL_IO_PORT;
> +	up->uartclk = SPRD_DEF_RATE;
> +	up->fifosize = SPRD_FIFO_SIZE;
> +	up->ops = &serial_sprd_ops;
> +	up->flags = UPF_BOOT_AUTOCONF;
> +
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (!IS_ERR(clk))
> +		up->uartclk = clk_get_rate(clk);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "not provide mem resource\n");
> +		return -ENODEV;
> +	}

This check is not required. It will be done by devm_ioremap_resource()

> +	up->mapbase = res->start;
> +	up->membase = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(up->membase))
> +		return PTR_ERR(up->membase);
> +
>

-- 
Thanks,
Varka Bhadram.

[PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support

[Varka Bhadram]

On Thursday 29 January 2015 08:56 PM, Varka Bhadram wrote:
> Hi,
>
> On Wednesday 28 January 2015 04:38 PM, Chunyan Zhang wrote:
>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>> spreadtrum sharkl64 platform.
>> This driver also support earlycon.
>>
>> Originally-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
>> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
>> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>> Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>   drivers/tty/serial/Kconfig       |   18 +
>>   drivers/tty/serial/Makefile      |    1 +
>>   drivers/tty/serial/sprd_serial.c |  793 
>> ++++++++++++++++++++++++++++++++++++++
>>   include/uapi/linux/serial_core.h |    3 +
>>   4 files changed, 815 insertions(+)
>>   create mode 100644 drivers/tty/serial/sprd_serial.c
>>
> (...)
>
>> +static int sprd_probe(struct platform_device *pdev)
>> +{
>> +    struct resource *res;
>> +    struct uart_port *up;
>> +    struct clk *clk;
>> +    int irq;
>> +    int index;
>> +    int ret;
>> +
>> +    for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
>> +        if (sprd_port[index] == NULL)
>> +            break;
>> +
>> +    if (index == ARRAY_SIZE(sprd_port))
>> +        return -EBUSY;
>> +
>> +    index = sprd_probe_dt_alias(index, &pdev->dev);
>> +
>> +    sprd_port[index] = devm_kzalloc(&pdev->dev,
>> +        sizeof(*sprd_port[index]), GFP_KERNEL);
>> +    if (!sprd_port[index])
>> +        return -ENOMEM;
>> +
>> +    up = &sprd_port[index]->port;
>> +    up->dev = &pdev->dev;
>> +    up->line = index;
>> +    up->type = PORT_SPRD;
>> +    up->iotype = SERIAL_IO_PORT;
>> +    up->uartclk = SPRD_DEF_RATE;
>> +    up->fifosize = SPRD_FIFO_SIZE;
>> +    up->ops = &serial_sprd_ops;
>> +    up->flags = UPF_BOOT_AUTOCONF;
>> +
>> +    clk = devm_clk_get(&pdev->dev, NULL);
>> +    if (!IS_ERR(clk))
>> +        up->uartclk = clk_get_rate(clk);
>> +
>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +    if (!res) {
>> +        dev_err(&pdev->dev, "not provide mem resource\n");
>> +        return -ENODEV;
>> +    }
>
> This check is not required. It will be done by devm_ioremap_resource()
>
>> +    up->mapbase = res->start;

Accessing of 'res' has to be done after devm_ioremap_resource()

>> +    up->membase = devm_ioremap_resource(&pdev->dev, res);
>> +    if (IS_ERR(up->membase))
>> +        return PTR_ERR(up->membase);
>> +
>>
>

-- 
Thanks,
Varka Bhadram.

[PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support

[Peter Hurley]

Hi Varka,

On 01/29/2015 10:26 AM, Varka Bhadram wrote:
> Hi,
> 
> On Wednesday 28 January 2015 04:38 PM, Chunyan Zhang wrote:
>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>> spreadtrum sharkl64 platform.
>> This driver also support earlycon.
>>
>> Originally-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
>> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
>> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>> Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>   drivers/tty/serial/Kconfig       |   18 +
>>   drivers/tty/serial/Makefile      |    1 +
>>   drivers/tty/serial/sprd_serial.c |  793 ++++++++++++++++++++++++++++++++++++++
>>   include/uapi/linux/serial_core.h |    3 +
>>   4 files changed, 815 insertions(+)
>>   create mode 100644 drivers/tty/serial/sprd_serial.c
>>
> (...)
> 
>> +static int sprd_probe(struct platform_device *pdev)
>> +{
>> +    struct resource *res;
>> +    struct uart_port *up;
>> +    struct clk *clk;
>> +    int irq;
>> +    int index;
>> +    int ret;
>> +
>> +    for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
>> +        if (sprd_port[index] == NULL)
>> +            break;
>> +
>> +    if (index == ARRAY_SIZE(sprd_port))
>> +        return -EBUSY;
>> +
>> +    index = sprd_probe_dt_alias(index, &pdev->dev);
>> +
>> +    sprd_port[index] = devm_kzalloc(&pdev->dev,
>> +        sizeof(*sprd_port[index]), GFP_KERNEL);
>> +    if (!sprd_port[index])
>> +        return -ENOMEM;
>> +
>> +    up = &sprd_port[index]->port;
>> +    up->dev = &pdev->dev;
>> +    up->line = index;
>> +    up->type = PORT_SPRD;
>> +    up->iotype = SERIAL_IO_PORT;
>> +    up->uartclk = SPRD_DEF_RATE;
>> +    up->fifosize = SPRD_FIFO_SIZE;
>> +    up->ops = &serial_sprd_ops;
>> +    up->flags = UPF_BOOT_AUTOCONF;
>> +
>> +    clk = devm_clk_get(&pdev->dev, NULL);
>> +    if (!IS_ERR(clk))
>> +        up->uartclk = clk_get_rate(clk);
>> +
>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +    if (!res) {
>> +        dev_err(&pdev->dev, "not provide mem resource\n");
>> +        return -ENODEV;
>> +    }
> 
> This check is not required. It will be done by devm_ioremap_resource()

I disagree. devm_ioremap_resource() interprets the NULL resource as
a bad parameter and returns -EINVAL which is then forwarded as the
return value from the probe.

-ENODEV is the correct return value from the probe if the expected
resource is not available (either because it doesn't exist or was already
claimed by another driver).

Regards,
Peter Hurley

>> +    up->mapbase = res->start;
>> +    up->membase = devm_ioremap_resource(&pdev->dev, res);
>> +    if (IS_ERR(up->membase))
>> +        return PTR_ERR(up->membase);
>> +
>>
> 

[PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support

[Russell King - ARM Linux]

On Thu, Jan 29, 2015 at 10:49:34AM -0500, Peter Hurley wrote:
> Hi Varka,
> 
> On 01/29/2015 10:26 AM, Varka Bhadram wrote:
> > This check is not required. It will be done by devm_ioremap_resource()
> 
> I disagree. devm_ioremap_resource() interprets the NULL resource as
> a bad parameter and returns -EINVAL which is then forwarded as the
> return value from the probe.
> 
> -ENODEV is the correct return value from the probe if the expected
> resource is not available (either because it doesn't exist or was already
> claimed by another driver).

(Adding Thierry as the author of this function.)

I believe devm_ioremap_resource() was explicitly designed to remove such
error handling from drivers, and to give drivers a unified error response
to such things as missing resources.

See the comments for this function in lib/devres.c.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support

[Peter Hurley]

On 01/29/2015 11:05 AM, Russell King - ARM Linux wrote:
> On Thu, Jan 29, 2015 at 10:49:34AM -0500, Peter Hurley wrote:
>> Hi Varka,
>>
>> On 01/29/2015 10:26 AM, Varka Bhadram wrote:
>>> This check is not required. It will be done by devm_ioremap_resource()
>>
>> I disagree. devm_ioremap_resource() interprets the NULL resource as
>> a bad parameter and returns -EINVAL which is then forwarded as the
>> return value from the probe.
>>
>> -ENODEV is the correct return value from the probe if the expected
>> resource is not available (either because it doesn't exist or was already
>> claimed by another driver).
> 
> (Adding Thierry as the author of this function.)
> 
> I believe devm_ioremap_resource() was explicitly designed to remove such
> error handling from drivers, and to give drivers a unified error response
> to such things as missing resources.
> 
> See the comments for this function in lib/devres.c.

Maybe the purpose would be better served by wrapping the idiom in a single
function.

Regards,
Peter Hurley

[PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support

[Thierry Reding]

On Thu, Jan 29, 2015 at 04:05:53PM +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 29, 2015 at 10:49:34AM -0500, Peter Hurley wrote:
> > Hi Varka,
> > 
> > On 01/29/2015 10:26 AM, Varka Bhadram wrote:
> > > This check is not required. It will be done by devm_ioremap_resource()
> > 
> > I disagree. devm_ioremap_resource() interprets the NULL resource as
> > a bad parameter and returns -EINVAL which is then forwarded as the
> > return value from the probe.
> > 
> > -ENODEV is the correct return value from the probe if the expected
> > resource is not available (either because it doesn't exist or was already
> > claimed by another driver).
> 
> (Adding Thierry as the author of this function.)
> 
> I believe devm_ioremap_resource() was explicitly designed to remove such
> error handling from drivers, and to give drivers a unified error response
> to such things as missing resources.
> 
> See the comments for this function in lib/devres.c.

Right. Before the introduction of this function drivers would copy the
same boilerplate but would use completely inconsistent return codes.
Well, to be correct devm_request_and_ioremap() was introduced first to
reduce the boilerplate, but one of the problems was that it returned a
NULL pointer on failure, so it was impossible to distinguish between
specific error conditions. It also had the negative side-effect of not
giving drivers any directions on what to do with the NULL return value
other than the example in the kerneldoc. But most people just didn't
seem to look at that, so instead of using -EADDRNOTAVAIL they'd again
go and do completely inconsistent things everywhere.

When we introduced devm_ioremap_resource(), the idea was to remove any
of these inconsistencies once and for all. You call the function and if
it fails you simply propagate the error code, so you get consistent
behaviour everywhere.

If I remember correctly the error codes for the various conditions were
discussed quite extensively, and what we currently have is what we got
by concensus.

-ENODEV is certainly not the correct return value if a resource is not
available. It translates to "no such device", but the device must
clearly be there, otherwise the ->probe() shouldn't have been called.
Or if it really isn't there, then you'd at least need a memory region
to probe, otherwise you can't determine whether it's there or not. So
from the perspective of a device driver I think a missing, or NULL,
resource, is indeed an "invalid argument".

I understand that people might see some ambiguity there, and -EINVAL is
certainly not a very accurate description, but such is the nature of
error codes. You pick what fits best. -ENXIO and -EADDRNOTAVAIL had been
under discussion as well if I remember correctly, but they both equally
ambiguous. -ENODATA might have been a better fit, but that too has a
different meaning in other contexts.

Besides, there's an error message that goes along with the error code
return, that should remove any ambiguity for people looking at dmesg.

All of that said, the original assertion that the check is not required
is still valid. We can argue at length about the specific error code but
if we decide that it needs to change, then we should modify
devm_ioremap_resource() rather than requiring all callers to perform the
extra check again.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150130/74abee2a/attachment.sig>

[PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support

[Peter Hurley]

On 01/30/2015 05:18 AM, Thierry Reding wrote:
> On Thu, Jan 29, 2015 at 04:05:53PM +0000, Russell King - ARM Linux wrote:
>> On Thu, Jan 29, 2015 at 10:49:34AM -0500, Peter Hurley wrote:
>>> Hi Varka,
>>>
>>> On 01/29/2015 10:26 AM, Varka Bhadram wrote:
>>>> This check is not required. It will be done by devm_ioremap_resource()
>>>
>>> I disagree. devm_ioremap_resource() interprets the NULL resource as
>>> a bad parameter and returns -EINVAL which is then forwarded as the
>>> return value from the probe.
>>>
>>> -ENODEV is the correct return value from the probe if the expected
>>> resource is not available (either because it doesn't exist or was already
>>> claimed by another driver).
>>
>> (Adding Thierry as the author of this function.)
>>
>> I believe devm_ioremap_resource() was explicitly designed to remove such
>> error handling from drivers, and to give drivers a unified error response
>> to such things as missing resources.
>>
>> See the comments for this function in lib/devres.c.
> 
> Right. Before the introduction of this function drivers would copy the
> same boilerplate but would use completely inconsistent return codes.
> Well, to be correct devm_request_and_ioremap() was introduced first to
> reduce the boilerplate, but one of the problems was that it returned a
> NULL pointer on failure, so it was impossible to distinguish between
> specific error conditions. It also had the negative side-effect of not
> giving drivers any directions on what to do with the NULL return value
> other than the example in the kerneldoc. But most people just didn't
> seem to look at that, so instead of using -EADDRNOTAVAIL they'd again
> go and do completely inconsistent things everywhere.
> 
> When we introduced devm_ioremap_resource(), the idea was to remove any
> of these inconsistencies once and for all. You call the function and if
> it fails you simply propagate the error code, so you get consistent
> behaviour everywhere.
> 
> If I remember correctly the error codes for the various conditions were
> discussed quite extensively, and what we currently have is what we got
> by concensus.
> 
> -ENODEV is certainly not the correct return value if a resource is not
> available. It translates to "no such device", but the device must
> clearly be there, otherwise the ->probe() shouldn't have been called.

-ENODEV is the appropriate return from the probe(); there is no device.
That returning that value doesn't make sense from devm_ioremap_resource
is simply a reflection of the awkward construct.

> Or if it really isn't there, then you'd at least need a memory region
> to probe, otherwise you can't determine whether it's there or not. So
> from the perspective of a device driver I think a missing, or NULL,
> resource, is indeed an "invalid argument".

Trying to argue that a missing host bus window is an "invalid argument"
to probe() is just trying to make the shoe fit.

> I understand that people might see some ambiguity there, and -EINVAL is
> certainly not a very accurate description, but such is the nature of
> error codes. You pick what fits best. -ENXIO and -EADDRNOTAVAIL had been
> under discussion as well if I remember correctly, but they both equally
> ambiguous. -ENODATA might have been a better fit, but that too has a
> different meaning in other contexts.
> 
> Besides, there's an error message that goes along with the error code
> return, that should remove any ambiguity for people looking at dmesg.
> 
> All of that said, the original assertion that the check is not required
> is still valid. We can argue at length about the specific error code but
> if we decide that it needs to change, then we should modify
> devm_ioremap_resource() rather than requiring all callers to perform the
> extra check again.

None of the devm_ioremap_resource() changes have been submitted for
serial drivers. I see no problem with accepting the Spreadtrum driver
as is, and if someone wants to do a massive changeset to rework the
platform_get_resource()/devm_ioremap_resource() idiom for serial drivers
for 3.21, then the Spreadtrum driver would be included then.

That said, I'd prefer to see that idiom wrapped in a single function
rather than what's being suggested.

Regards,
Peter Hurley

[PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support

[Russell King - ARM Linux]

On Fri, Jan 30, 2015 at 07:03:03AM -0500, Peter Hurley wrote:
> On 01/30/2015 05:18 AM, Thierry Reding wrote:
> > -ENODEV is certainly not the correct return value if a resource is not
> > available. It translates to "no such device", but the device must
> > clearly be there, otherwise the ->probe() shouldn't have been called.
> 
> -ENODEV is the appropriate return from the probe(); there is no device.

No it is not.  "no such device" means "the device is not present".  If
the device is not present, we wouldn't have a struct device associated
with it.

The missing resource is an error condition: what it's saying is that the
device is there, but something has failed in providing the IO regions
necessary to access it.  That's really something very different from
"there is no device present".

> > Or if it really isn't there, then you'd at least need a memory region
> > to probe, otherwise you can't determine whether it's there or not. So
> > from the perspective of a device driver I think a missing, or NULL,
> > resource, is indeed an "invalid argument".
> 
> Trying to argue that a missing host bus window is an "invalid argument"
> to probe() is just trying to make the shoe fit.

As is arguing that -ENODEV is an appropriate return value for the missing
resource.

Moreover, returning -ENODEV is actually *bad* in this case - the kernel's
generic probe handling does not report the failure of the driver to bind
given this, so a missing resource potentially becomes a silent failure of
a driver - leading users to wonder why their devices aren't found.

If we /really/ have a problem with the error code, then why not invent
a new error code to cater for this condition - maybe, EBADRES (bad resource).

> > I understand that people might see some ambiguity there, and -EINVAL is
> > certainly not a very accurate description, but such is the nature of
> > error codes. You pick what fits best. -ENXIO and -EADDRNOTAVAIL had been
> > under discussion as well if I remember correctly, but they both equally
> > ambiguous. -ENODATA might have been a better fit, but that too has a
> > different meaning in other contexts.
> > 
> > Besides, there's an error message that goes along with the error code
> > return, that should remove any ambiguity for people looking at dmesg.
> > 
> > All of that said, the original assertion that the check is not required
> > is still valid. We can argue at length about the specific error code but
> > if we decide that it needs to change, then we should modify
> > devm_ioremap_resource() rather than requiring all callers to perform the
> > extra check again.
> 
> None of the devm_ioremap_resource() changes have been submitted for
> serial drivers.

$ grep devm_ioremap_resource drivers/tty/serial/ -r | wc -l
10

It seems that statement is false.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support

[Peter Hurley]

On 01/30/2015 09:08 AM, Russell King - ARM Linux wrote:
> On Fri, Jan 30, 2015 at 07:03:03AM -0500, Peter Hurley wrote:
>> On 01/30/2015 05:18 AM, Thierry Reding wrote:
>>> -ENODEV is certainly not the correct return value if a resource is not
>>> available. It translates to "no such device", but the device must
>>> clearly be there, otherwise the ->probe() shouldn't have been called.
>>
>> -ENODEV is the appropriate return from the probe(); there is no device.
> 
> No it is not.  "no such device" means "the device is not present".  If
> the device is not present, we wouldn't have a struct device associated
> with it.
> 
> The missing resource is an error condition: what it's saying is that the
> device is there, but something has failed in providing the IO regions
> necessary to access it.  That's really something very different from
> "there is no device present".

This is masking behavior changes behind what is essentially a refactor.
For example, here's Felipe's changelog to omap-serial:

commit d044d2356f8dd18c755e13f34318bc03ef9c6887
Author: Felipe Balbi <balbi@ti.com>
Date:   Wed Apr 23 09:58:33 2014 -0500

    tty: serial: omap: switch over to devm_ioremap_resource
    
    just using helper function to remove some duplicated
    code a bit. While at that, also move allocation of
    struct uart_omap_port higher in the code so that
    we return much earlier in case of no memory.
    
    Signed-off-by: Felipe Balbi <balbi@ti.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

No mention of correcting an error return value here.

Why change the error return from probe() now?

Before you say consistency, I think you should look at the stats below.
IOW, if you want to change the error code return from probe() for
consistency's sake, a tree-wide patch would be the appropriate way.


>>> Or if it really isn't there, then you'd at least need a memory region
>>> to probe, otherwise you can't determine whether it's there or not. So
>>> from the perspective of a device driver I think a missing, or NULL,
>>> resource, is indeed an "invalid argument".
>>
>> Trying to argue that a missing host bus window is an "invalid argument"
>> to probe() is just trying to make the shoe fit.
> 
> As is arguing that -ENODEV is an appropriate return value for the missing
> resource.
> 
> Moreover, returning -ENODEV is actually *bad* in this case - the kernel's
> generic probe handling does not report the failure of the driver to bind
> given this, so a missing resource potentially becomes a silent failure of
> a driver - leading users to wonder why their devices aren't found.
> 
> If we /really/ have a problem with the error code, then why not invent
> a new error code to cater for this condition - maybe, EBADRES (bad resource).
> 
>>> I understand that people might see some ambiguity there, and -EINVAL is
>>> certainly not a very accurate description, but such is the nature of
>>> error codes. You pick what fits best. -ENXIO and -EADDRNOTAVAIL had been
>>> under discussion as well if I remember correctly, but they both equally
>>> ambiguous. -ENODATA might have been a better fit, but that too has a
>>> different meaning in other contexts.
>>>
>>> Besides, there's an error message that goes along with the error code
>>> return, that should remove any ambiguity for people looking at dmesg.
>>>
>>> All of that said, the original assertion that the check is not required
>>> is still valid. We can argue at length about the specific error code but
>>> if we decide that it needs to change, then we should modify
>>> devm_ioremap_resource() rather than requiring all callers to perform the
>>> extra check again.
>>
>> None of the devm_ioremap_resource() changes have been submitted for
>> serial drivers.
> 
> $ grep devm_ioremap_resource drivers/tty/serial/ -r | wc -l
> 10

Ok, not 'none' but hardly tree-wide.

And of those 10 drivers now using devm_ioremap_resource(),
3 drivers still return ENODEV for a missing resource [1]. (FWIW, I wrote 'none'
on the basis of a grep of devm_ioremap_resource and looking at the last one,
serial-tegra.c, which has exactly the construct objected to in the Spreadtrum
driver).

Another 9 drivers still use plain devm_ioremap(), even those with
trivial conversions like samsung.c [2]

Of the serial drivers which use platform_get_resource(),
10 return  ENODEV
5  return  EINVAL (not including those converted to devm_ioremap_resource())
4  return  ENXIO
3  return  ENOMEM
2  return  ENOENT
1  returns EBUSY
1  returns EFAULT

So to recap, I see no reason to respin this driver submission when:
1. even drivers already using devm_ioremap_resource() aren't consistent
2. drivers which could trivially use devm_ioremap_resource, don't
3. there's no basis for requiring consistent return value _yet_
   (or even what that value should be)
4. the platform_get_resource()/devm_ioremap_resource is an awkward code construct

Regards,
Peter Hurley


[1]
drivers/tty/serial/bcm63xx_uart.c-	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
drivers/tty/serial/bcm63xx_uart.c-	if (!res_mem)
drivers/tty/serial/bcm63xx_uart.c-		return -ENODEV;
drivers/tty/serial/bcm63xx_uart.c-
drivers/tty/serial/bcm63xx_uart.c-	port->mapbase = res_mem->start;
drivers/tty/serial/bcm63xx_uart.c:	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);

drivers/tty/serial/vt8500_serial.c-	mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
drivers/tty/serial/vt8500_serial.c-	irqres = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
drivers/tty/serial/vt8500_serial.c-	if (!mmres || !irqres)
drivers/tty/serial/vt8500_serial.c-		return -ENODEV;
[...]
drivers/tty/serial/vt8500_serial.c:	vt8500_port->uart.membase = devm_ioremap_resource(&pdev->dev, mmres);

drivers/tty/serial/serial-tegra.c-	resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
drivers/tty/serial/serial-tegra.c-	if (!resource) {
drivers/tty/serial/serial-tegra.c-		dev_err(&pdev->dev, "No IO memory resource\n");
drivers/tty/serial/serial-tegra.c-		return -ENODEV;
drivers/tty/serial/serial-tegra.c-	}
drivers/tty/serial/serial-tegra.c-
drivers/tty/serial/serial-tegra.c-	u->mapbase = resource->start;
drivers/tty/serial/serial-tegra.c:	u->membase = devm_ioremap_resource(&pdev->dev, resource);


[2]
drivers/tty/serial/samsung.c:	res = platform_get_resource(platdev, IORESOURCE_MEM, 0);
drivers/tty/serial/samsung.c-	if (res == NULL) {
drivers/tty/serial/samsung.c-		dev_err(port->dev, "failed to find memory resource for uart\n");
drivers/tty/serial/samsung.c-		return -EINVAL;
drivers/tty/serial/samsung.c-	}
drivers/tty/serial/samsung.c-
drivers/tty/serial/samsung.c-	dbg("resource %pR)\n", res);
drivers/tty/serial/samsung.c-
drivers/tty/serial/samsung.c-	port->membase = devm_ioremap(port->dev, res->start, resource_size(res));
drivers/tty/serial/samsung.c-	if (!port->membase) {
drivers/tty/serial/samsung.c-		dev_err(port->dev, "failed to remap controller address\n");

[PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support

[Russell King - ARM Linux]

On Fri, Jan 30, 2015 at 10:32:54AM -0500, Peter Hurley wrote:
> Before you say consistency, I think you should look at the stats below.
> IOW, if you want to change the error code return from probe() for
> consistency's sake, a tree-wide patch would be the appropriate way.

Now look outside the serial driver sub-tree.

There are 1234 instances of platform_get_resource(, IORESOURCE_MEM, ) in
the drivers/ sub-tree, with 700 instances of devm_ioremap_resource()
being used there.  Of the devm_ioremap_resource() instances:

- 555 use platform_get_resource() in the preceding two lines - which is
  not enough to do anything but rely on the -EINVAL return value.
- 16 mention ENODEV in the preceding three lines.

There are 132 which use platform_get_resource() and return ENODEV within
the following three lines (which may intersect with the above 16 number)
and 88 which use EINVAL.

So, there are in total 643 instances where a missing resource returns
EINVAL, and between 132 and 148 instances which return ENODEV.

Yes, 643 + 148 isn't 1234, but I'm not going to read through all 1234
locations just for the sake of this thread.   What's clear though is that
more than 50% of sites using platform_get_resource(, IORESOURCE_MEM, )
return EINVAL for the lack of a resource.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support

[Peter Hurley]

On 01/30/2015 10:49 AM, Russell King - ARM Linux wrote:
> On Fri, Jan 30, 2015 at 10:32:54AM -0500, Peter Hurley wrote:
>> Before you say consistency, I think you should look at the stats below.
>> IOW, if you want to change the error code return from probe() for
>> consistency's sake, a tree-wide patch would be the appropriate way.
> 
> Now look outside the serial driver sub-tree.
> 
> There are 1234 instances of platform_get_resource(, IORESOURCE_MEM, ) in
> the drivers/ sub-tree, with 700 instances of devm_ioremap_resource()
> being used there.  Of the devm_ioremap_resource() instances:
> 
> - 555 use platform_get_resource() in the preceding two lines - which is
>   not enough to do anything but rely on the -EINVAL return value.
> - 16 mention ENODEV in the preceding three lines.
> 
> There are 132 which use platform_get_resource() and return ENODEV within
> the following three lines (which may intersect with the above 16 number)
> and 88 which use EINVAL.
> 
> So, there are in total 643 instances where a missing resource returns
> EINVAL, and between 132 and 148 instances which return ENODEV.
> 
> Yes, 643 + 148 isn't 1234, but I'm not going to read through all 1234
> locations just for the sake of this thread.   What's clear though is that
> more than 50% of sites using platform_get_resource(, IORESOURCE_MEM, )
> return EINVAL for the lack of a resource.

Sure, now that they're using devm_ioremap_resource(). What about before
they were converted?

For example, of the 10 serial drivers now using devm_ioremap_resource(),
_not 1 returned EINVAL_ prior to using devm_ioremap_resource(). And of those
10 commits, only 1 mentions changing the return codes on purpose.

In fact, all of them but one returned ENODEV.

Regards,
Peter Hurley

[PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support

[Peter Hurley]

On 01/29/2015 11:04 AM, Varka Bhadram wrote:
> Hi Peter,
> 
> On Thursday 29 January 2015 09:19 PM, Peter Hurley wrote:
>> Hi Varka,
>>
>> On 01/29/2015 10:26 AM, Varka Bhadram wrote:
>>> Hi,
>>>
>>> On Wednesday 28 January 2015 04:38 PM, Chunyan Zhang wrote:
>>>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>>>> spreadtrum sharkl64 platform.
>>>> This driver also support earlycon.
>>>>
>>>> Originally-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
>>>> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
>>>> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
>>>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>>>> Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
>>>> ---
>>>>   drivers/tty/serial/Kconfig       |   18 +
>>>>   drivers/tty/serial/Makefile      |    1 +
>>>>   drivers/tty/serial/sprd_serial.c |  793 ++++++++++++++++++++++++++++++++++++++
>>>>   include/uapi/linux/serial_core.h |    3 +
>>>>   4 files changed, 815 insertions(+)
>>>>   create mode 100644 drivers/tty/serial/sprd_serial.c
>>>>
>>> (...)
>>>
>>>> +static int sprd_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct resource *res;
>>>> +    struct uart_port *up;
>>>> +    struct clk *clk;
>>>> +    int irq;
>>>> +    int index;
>>>> +    int ret;
>>>> +
>>>> +    for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
>>>> +        if (sprd_port[index] == NULL)
>>>> +            break;
>>>> +
>>>> +    if (index == ARRAY_SIZE(sprd_port))
>>>> +        return -EBUSY;
>>>> +
>>>> +    index = sprd_probe_dt_alias(index, &pdev->dev);
>>>> +
>>>> +    sprd_port[index] = devm_kzalloc(&pdev->dev,
>>>> +        sizeof(*sprd_port[index]), GFP_KERNEL);
>>>> +    if (!sprd_port[index])
>>>> +        return -ENOMEM;
>>>> +
>>>> +    up = &sprd_port[index]->port;
>>>> +    up->dev = &pdev->dev;
>>>> +    up->line = index;
>>>> +    up->type = PORT_SPRD;
>>>> +    up->iotype = SERIAL_IO_PORT;
>>>> +    up->uartclk = SPRD_DEF_RATE;
>>>> +    up->fifosize = SPRD_FIFO_SIZE;
>>>> +    up->ops = &serial_sprd_ops;
>>>> +    up->flags = UPF_BOOT_AUTOCONF;
>>>> +
>>>> +    clk = devm_clk_get(&pdev->dev, NULL);
>>>> +    if (!IS_ERR(clk))
>>>> +        up->uartclk = clk_get_rate(clk);
>>>> +
>>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +    if (!res) {
>>>> +        dev_err(&pdev->dev, "not provide mem resource\n");
>>>> +        return -ENODEV;
>>>> +    }
>>> This check is not required. It will be done by devm_ioremap_resource()
>> I disagree. devm_ioremap_resource() interprets the NULL resource as
>> a bad parameter and returns -EINVAL which is then forwarded as the
>> return value from the probe.
>>
>> -ENODEV is the correct return value from the probe if the expected
>> resource is not available (either because it doesn't exist or was already
>> claimed by another driver).
> 
> Check on the resource happening with evm_ioremap_resource.
> 
> Not necessary to check multiple times.
> 
> I did series for all the drivers. see [1]
> 
> [1]: https://lkml.org/lkml/2014/11/3/986 <https://lkml.org/lkml/2014/11/3/986>

That's a usb series. Did you do a serial driver series I missed?
I don't see anything related in Greg's tty-next tree...

Regards,
Peter Hurley