LinuxLists.cc - [PATCH v3 0/5] Add Spreadtrum Sharkl64 Platform support

[permalink] [raw]

Subject: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

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

Signed-off-by: Chunyan Zhang <[email protected]>
Signed-off-by: Orson Zhai <[email protected]>
Originally-by: Lanqing Liu <[email protected]>
---
Documentation/devices.txt | 3 +
drivers/tty/serial/Kconfig | 23 ++
drivers/tty/serial/Makefile | 1 +
drivers/tty/serial/sprd_serial.c | 752 ++++++++++++++++++++++++++++++++++++++
include/uapi/linux/serial_core.h | 3 +
5 files changed, 782 insertions(+)
create mode 100644 drivers/tty/serial/sprd_serial.c

diff --git a/Documentation/devices.txt b/Documentation/devices.txt
index 87b4c5e..1da0432 100644
--- a/Documentation/devices.txt
+++ b/Documentation/devices.txt
@@ -2816,6 +2816,9 @@ Your cooperation is appreciated.
210 = /dev/ttyMAX1 MAX3100 serial port 1
211 = /dev/ttyMAX2 MAX3100 serial port 2
212 = /dev/ttyMAX3 MAX3100 serial port 3
+ 213 = /dev/ttySPX0 SPRD serial port 0
+ ...
+ 216 = /dev/ttySPX3 SPRD serial port 3

205 char Low-density serial ports (alternate device)
0 = /dev/culu0 Callout device for ttyLU0
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 649b784..2c2cf60 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1573,6 +1573,29 @@ 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 SPRD serial"
+ depends on ARCH_SPRD
+ select SERIAL_CORE
+ help
+ This enables the driver for the Spreadtrum's serial.
+
+config SERIAL_SPRD_NR
+ int "Maximum number of sprd serial ports"
+ depends on SERIAL_SPRD
+ default "4"
+
+config SERIAL_SPRD_CONSOLE
+ bool "SPRD 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..58214c8
--- /dev/null
+++ b/drivers/tty/serial/sprd_serial.c
@@ -0,0 +1,752 @@
+/*
+ * Copyright (C) 2012 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.
+ */
+
+#include <linux/module.h>
+#include <linux/tty.h>
+#include <linux/ioport.h>
+#include <linux/console.h>
+#include <linux/platform_device.h>
+#include <linux/tty_flip.h>
+#include <linux/serial_core.h>
+#include <linux/serial.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <asm/irq.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+
+/* device name */
+#define UART_NR_MAX CONFIG_SERIAL_SPRD_NR
+#define SPRD_TTY_NAME "ttySPX"
+#define SPRD_TTY_MAJOR 204
+#define SPRD_TTY_MINOR_START 213
+#define SPRD_FIFO_SIZE 128
+#define SPRD_DEF_RATE 26000000
+
+/* 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)
+
+/* 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 {
+ uint32_t ien;
+ uint32_t ctrl0;
+ uint32_t ctrl1;
+ uint32_t ctrl2;
+ uint32_t clkd0;
+ uint32_t clkd1;
+ uint32_t 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] = { NULL };
+
+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 inline int handle_lsr_errors(struct uart_port *port,
+ unsigned int *flag, unsigned int *lsr)
+{
+ int ret = 0;
+
+ /* stastics */
+ 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(int irq, void *dev_id)
+{
+ struct uart_port *port = (struct uart_port *)dev_id;
+ struct tty_port *tty = &port->state->port;
+ unsigned int ch, flag, lsr, max_count = 2048;
+
+ 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 (unlikely(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(int irq, void *dev_id)
+{
+ struct uart_port *port = dev_id;
+ 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 = (struct uart_port *)dev_id;
+ u32 ims;
+
+ ims = serial_in(port, SPRD_IMSR);
+
+ serial_out(port, SPRD_ICLR, ~0);
+
+ if (ims & (SPRD_IMSR_RX_FIFO_FULL |
+ SPRD_IMSR_BREAK_DETECT | SPRD_IMSR_TIMEOUT)) {
+ sprd_rx(irq, port);
+ }
+ if (ims & SPRD_IMSR_TX_FIFO_EMPTY)
+ sprd_tx(irq, port);
+
+ return IRQ_HANDLED;
+}
+
+static int sprd_startup(struct uart_port *port)
+{
+ int ret = 0;
+ unsigned int ien, ctrl1;
+ struct sprd_uart_port *sp;
+
+ serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY << 8) | THLD_RX_FULL));
+
+ /* clear rx fifo */
+ while (serial_in(port, SPRD_STS1) & 0x00ff)
+ serial_in(port, SPRD_RXD);
+
+ /* clear tx fifo */
+ while (serial_in(port, SPRD_STS1) & 0xff00)
+ ;
+
+ /* clear interrupt */
+ serial_out(port, SPRD_IEN, 0x0);
+ 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\n",
+ port->irq);
+ return ret;
+ }
+ ctrl1 = serial_in(port, SPRD_CTL1);
+ ctrl1 |= 0x3e00 | THLD_RX_FULL;
+ serial_out(port, SPRD_CTL1, ctrl1);
+
+ /* enable interrupt */
+ spin_lock(&port->lock);
+ 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(&port->lock);
+
+ return 0;
+}
+
+static void sprd_shutdown(struct uart_port *port)
+{
+ serial_out(port, SPRD_IEN, 0x0);
+ 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, fc;
+
+ /* ask the core to calculate the divisor for us */
+ baud = uart_get_baud_rate(port, termios, old, 1200, 3000000);
+
+ quot = (unsigned int)((port->uartclk + baud / 2) / baud);
+
+ /* set data length */
+ lcr = serial_in(port, SPRD_LCR);
+ lcr &= ~SPRD_LCR_DATA_LEN;
+ 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;
+ 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;
+ }
+
+ /* change the port state. */
+ /* 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 & (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 |= 0x3e00 | THLD_RX_FULL;
+ serial_out(port, SPRD_CTL1, fc);
+}
+
+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 (unlikely(ser->type != PORT_SPRD))
+ return -EINVAL;
+ if (unlikely(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 = (struct uart_port *)sprd_port[co->index];
+ int ien;
+ int locked = 1;
+
+ if (oops_in_progress)
+ locked = spin_trylock(&port->lock);
+ else
+ spin_lock(&port->lock);
+ /* save the IEN then disable the interrupts */
+ ien = serial_in(port, SPRD_IEN);
+ serial_out(port, SPRD_IEN, 0x0);
+
+ uart_console_write(port, s, count, sprd_console_putchar);
+
+ /* wait for transmitter to become empty and restore the IEN */
+ wait_for_xmitr(port);
+ serial_out(port, SPRD_IEN, ien);
+ if (locked)
+ spin_unlock(&port->lock);
+}
+
+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 (unlikely(co->index >= UART_NR_MAX || co->index < 0))
+ co->index = 0;
+
+ port = (struct uart_port *)sprd_port[co->index];
+ if (port == NULL) {
+ pr_info("srial 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)
+{
+ while (!(readl(port->membase + SPRD_LSR) & SPRD_LSR_TX_OVER))
+ ;
+ 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 = SPRD_TTY_MAJOR,
+ .minor = SPRD_TTY_MINOR_START,
+ .nr = UART_NR_MAX,
+ .cons = SPRD_CONSOLE,
+};
+
+static int sprd_probe(struct platform_device *pdev)
+{
+ struct resource *mem;
+ struct device_node *np = pdev->dev.of_node;
+ struct uart_port *up;
+ struct clk *clk;
+ int irq;
+
+
+ if (np)
+ pdev->id = of_alias_get_id(np, "serial");
+
+ if (unlikely(pdev->id < 0 || pdev->id >= UART_NR_MAX)) {
+ dev_err(&pdev->dev, "does not support id %d\n", pdev->id);
+ return -ENXIO;
+ }
+
+ sprd_port[pdev->id] = devm_kzalloc(&pdev->dev,
+ sizeof(*sprd_port[pdev->id]), GFP_KERNEL);
+ if (!sprd_port[pdev->id])
+ return -ENOMEM;
+
+ up = (struct uart_port *)sprd_port[pdev->id];
+ up->dev = &pdev->dev;
+ up->line = pdev->id;
+ 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 = ASYNC_BOOT_AUTOCONF;
+
+ clk = devm_clk_get(&pdev->dev, NULL);
+ if (!IS_ERR(clk))
+ up->uartclk = clk_get_rate(clk);
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (unlikely(!mem)) {
+ dev_err(&pdev->dev, "not provide mem resource\n");
+ return -ENODEV;
+ }
+ up->mapbase = mem->start;
+ up->membase = ioremap(mem->start, resource_size(mem));
+
+ irq = platform_get_irq(pdev, 0);
+ if (unlikely(irq < 0)) {
+ dev_err(&pdev->dev, "not provide irq resource\n");
+ return -ENODEV;
+ }
+ up->irq = irq;
+
+ platform_set_drvdata(pdev, up);
+
+ return uart_add_one_port(&sprd_uart_driver, up);
+}
+
+static int sprd_remove(struct platform_device *dev)
+{
+ struct uart_port *up = platform_get_drvdata(dev);
+
+ return uart_remove_one_port(&sprd_uart_driver, up);
+}
+
+static int sprd_suspend(struct platform_device *dev, pm_message_t state)
+{
+ int id = dev->id;
+ struct uart_port *port = (struct uart_port *)sprd_port[id];
+ struct reg_backup *reg_bak = &(sprd_port[id]->reg_bak);
+
+ reg_bak->ien = serial_in(port, SPRD_IEN);
+ reg_bak->ctrl0 = serial_in(port, SPRD_LCR);
+ reg_bak->ctrl1 = serial_in(port, SPRD_CTL1);
+ reg_bak->ctrl2 = serial_in(port, SPRD_CTL2);
+ reg_bak->clkd0 = serial_in(port, SPRD_CLKD0);
+ reg_bak->clkd1 = serial_in(port, SPRD_CLKD1);
+
+ return 0;
+}
+
+static int sprd_resume(struct platform_device *dev)
+{
+ int id = dev->id;
+ struct uart_port *port = (struct uart_port *)sprd_port[id];
+ struct reg_backup *reg_bak = &(sprd_port[id]->reg_bak);
+
+ serial_out(port, SPRD_LCR, reg_bak->ctrl0);
+ serial_out(port, SPRD_CTL1, reg_bak->ctrl1);
+ serial_out(port, SPRD_CTL2, reg_bak->ctrl2);
+ serial_out(port, SPRD_CLKD0, reg_bak->clkd0);
+ serial_out(port, SPRD_CLKD1, reg_bak->clkd1);
+ serial_out(port, SPRD_IEN, reg_bak->ien);
+
+ return 0;
+}
+
+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,
+ .suspend = sprd_suspend,
+ .resume = sprd_resume,
+ .driver = {
+ .name = "sprd_serial",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(serial_ids),
+ },
+};
+
+static int __init sprd_serial_init(void)
+{
+ int ret = 0;
+
+ ret = uart_register_driver(&sprd_uart_driver);
+ if (unlikely(ret != 0))
+ return ret;
+
+ ret = platform_driver_register(&sprd_platform_driver);
+ if (unlikely(ret != 0))
+ uart_unregister_driver(&sprd_uart_driver);
+
+ return ret;
+}
+
+static void __exit sprd_serial_exit(void)
+{
+ platform_driver_unregister(&sprd_platform_driver);
+ uart_unregister_driver(&sprd_uart_driver);
+}
+
+module_init(sprd_serial_init);
+module_exit(sprd_serial_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Spreadtrum SoC serial driver series");
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index 16ad852..d9a8c88 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -247,4 +247,7 @@
/* MESON */
#define PORT_MESON 109

+/* SPRD SERIAL */
+#define PORT_SPRD 110
+
#endif /* _UAPILINUX_SERIAL_CORE_H */
--
1.7.9.5

2014-11-25 12:40:54

[permalink] [raw]

Subject: [PATCH v3 2/5] Documentation: DT: Add bindings for Spreadtrum SoC Platform

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 <[email protected]>
Signed-off-by: Orson Zhai <[email protected]>
---
Documentation/devicetree/bindings/arm/sprd.txt | 11 +++++++++++
.../devicetree/bindings/serial/sprd-uart.txt | 6 ++++++
.../devicetree/bindings/vendor-prefixes.txt | 1 +
3 files changed, 18 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..54e532f
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/sprd-uart.txt
@@ -0,0 +1,6 @@
+* 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
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 723999d..ce99ecdfb 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -142,6 +142,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

2014-11-25 12:54:10

[permalink] [raw]

Subject: Re: [PATCH v3 2/5] Documentation: DT: Add bindings for Spreadtrum SoC Platform

On Tuesday 25 November 2014 20:16:55 Chunyan Zhang wrote:
> diff --git a/Documentation/devicetree/bindings/serial/sprd-uart.txt b/Documentation/devicetree/bindings/serial/sprd-uart.txt
> new file mode 100644
> index 0000000..54e532f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/sprd-uart.txt
> @@ -0,0 +1,6 @@
> +* 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
>

The driver uses a clock, and the dts file lists a clock
property, so it should be documented here, either as optional
or mandatory, and with a description what this clock refers to.

Arnd

2014-11-25 12:58:44

[permalink] [raw]

Subject: Re: [PATCH v3 4/5] arm64: Add support for Spreadtrum's Sharkl64 Platform in Kconfig and defconfig

On Tuesday 25 November 2014 20:16:57 Chunyan Zhang wrote:
>
> +menuconfig ARCH_SPRD
> + bool "Spreadtrum SoC platform"
> + depends on ARM64
> + help
> + Support for Spreadtrum ARM based SoCs
> +
> +if ARCH_SPRD
> +
> +config ARCH_SHARKL64
> + bool "Sharkl64 SoC Platform"
> + help
> + Sharkl64 is a Spreadtrum's SoC Platform which is based
> + on ARM 64-bit processor core including
> + sc9836
> +
> +endif #ARCH_SPRD
> +

I don't think we need multiple levels here, it should be enough to
have either ARCH_SPRD or ARCH_SHARKL64, because all device drivers
are going to be optional anyway. Typically a Kconfig symbol covers
all SoCs that are related, so if you Spreadtrum are doing both
phone and server chips and these are designed independently, you
would have two symbols, but if you only expect to see phone chips
here that are all derived from the same product line, using ARCH_SPRD
to refer to all of them should be enough.

Arnd

2014-11-25 13:00:21

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v3 0/5] Add Spreadtrum Sharkl64 Platform support

On Tue, Nov 25, 2014 at 08:16:53PM +0800, Chunyan Zhang wrote:
> Spreadtrum is a rapid growing chip vendor providing smart phone total solutions.
>
> Sharkl64 Platform is nominated as a SoC infrastructure that supports 4G/3G/2G
> standards based on ARMv8 multiple core architecture.Now we have only one
> SoC(SC9836) based on this Platform in developing.

This series is being sent to both my work and upstream addresses -
please don't do that, send it to just one (normally my upstream one for
upstream work).

Attachments:

(No filename) (512.00 B)
signature.asc (473.00 B)
Digital signature Download all attachments

2014-11-25 13:00:33

[permalink] [raw]

Subject: Re: [PATCH v3 0/5] Add Spreadtrum Sharkl64 Platform support

On Tuesday 25 November 2014 20:16:53 Chunyan Zhang wrote:
> Spreadtrum is a rapid growing chip vendor providing smart phone total solutions.
>
> Sharkl64 Platform is nominated as a SoC infrastructure that supports 4G/3G/2G
> standards based on ARMv8 multiple core architecture.Now we have only one
> SoC(SC9836) based on this Platform in developing.
>
> This patchset adds Sharkl64 support in arm64 device tree and the serial driver
> of SC9836-UART.
>
> This patchset also has patches which address "sprd" prefix and DT compatible
> strings for nodes which appear un-documented.
>
> This version code was tesed on Fast Mode.
> We use boot-wrapper-aarch64 as the bootloader.

Looks very good overall. I have two small comments to individual patches,
but I think we can still merge them for 3.19. Please send the serial
driver to Greg for inclusion, unless you get further comments, and send
the updated version of the other patches to [email protected] and ask
for including them in the cover letter.

Arnd

2014-11-25 13:08:09

[permalink] [raw]

Subject: [PATCH v3 4/5] arm64: Add support for Spreadtrum's Sharkl64 Platform in Kconfig and defconfig

From: Zhizhou Zhang <[email protected]>

Adds support for Spreadtrum's SoC Platform and its subset Sharkl64
in the arm64 Kconfig and defconfig files.

Signed-off-by: Zhizhou Zhang <[email protected]>
Signed-off-by: Chunyan Zhang <[email protected]>
Signed-off-by: Orson Zhai <[email protected]>
---
arch/arm64/Kconfig | 17 +++++++++++++++++
arch/arm64/configs/defconfig | 2 ++
2 files changed, 19 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 9532f8d..a63ec45 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -147,6 +147,23 @@ config ARCH_THUNDER
help
This enables support for Cavium's Thunder Family of SoCs.

+menuconfig ARCH_SPRD
+ bool "Spreadtrum SoC platform"
+ depends on ARM64
+ help
+ Support for Spreadtrum ARM based SoCs
+
+if ARCH_SPRD
+
+config ARCH_SHARKL64
+ bool "Sharkl64 SoC Platform"
+ help
+ Sharkl64 is a Spreadtrum's SoC Platform which is based
+ on ARM 64-bit processor core including
+ sc9836
+
+endif #ARCH_SPRD
+
config ARCH_VEXPRESS
bool "ARMv8 software model (Versatile Express)"
select ARCH_REQUIRE_GPIOLIB
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index dd301be..d7934a8 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -33,6 +33,8 @@ CONFIG_MODULE_UNLOAD=y
# CONFIG_BLK_DEV_BSG is not set
# CONFIG_IOSCHED_DEADLINE is not set
CONFIG_ARCH_THUNDER=y
+CONFIG_ARCH_SPRD=y
+CONFIG_ARCH_SHARKL64=y
CONFIG_ARCH_VEXPRESS=y
CONFIG_ARCH_XGENE=y
CONFIG_PCI=y
--
1.7.9.5

2014-11-25 13:08:32

[permalink] [raw]

Subject: [PATCH v3 1/5] Documentation: DT: Renamed of-serial.txt to 8250.txt

The file of-serial.txt was only for 8250 compatible UART implementations,
so renamed it to 8250.txt to avoid confusing other persons.
This is recommended by Arnd, see:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/291455.html

Signed-off-by: Chunyan Zhang <[email protected]>
---
.../bindings/serial/{of-serial.txt => 8250.txt} | 0
1 file changed, 0 insertions(+), 0 deletions(-)
rename Documentation/devicetree/bindings/serial/{of-serial.txt => 8250.txt} (100%)

diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/8250.txt
similarity index 100%
rename from Documentation/devicetree/bindings/serial/of-serial.txt
rename to Documentation/devicetree/bindings/serial/8250.txt
--
1.7.9.5

2014-11-25 13:09:09

[permalink] [raw]

Subject: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

From: Zhizhou Zhang <[email protected]>

Adds the device tree support for Spreadtrum SC9836 SoC which is based on
Sharkl64 platform.

Sharkl64 platform contains the common nodes of Spreadtrum's arm64-based SoCs.

Signed-off-by: Zhizhou Zhang <[email protected]>
Signed-off-by: Chunyan Zhang <[email protected]>
Signed-off-by: Orson Zhai <[email protected]>
---
arch/arm64/boot/dts/Makefile | 1 +
arch/arm64/boot/dts/sprd-sc9836-openphone.dts | 85 ++++++++++++++++++++
arch/arm64/boot/dts/sprd-sc9836.dtsi | 103 ++++++++++++++++++++++++
arch/arm64/boot/dts/sprd-sharkl64.dtsi | 105 +++++++++++++++++++++++++
4 files changed, 294 insertions(+)
create mode 100644 arch/arm64/boot/dts/sprd-sc9836-openphone.dts
create mode 100644 arch/arm64/boot/dts/sprd-sc9836.dtsi
create mode 100644 arch/arm64/boot/dts/sprd-sharkl64.dtsi

diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
index f8001a6..d0aff8a 100644
--- a/arch/arm64/boot/dts/Makefile
+++ b/arch/arm64/boot/dts/Makefile
@@ -1,4 +1,5 @@
dtb-$(CONFIG_ARCH_THUNDER) += thunder-88xx.dtb
+dtb-$(CONFIG_ARCH_SHARKL64) += sprd-sc9836-openphone.dtb
dtb-$(CONFIG_ARCH_VEXPRESS) += rtsm_ve-aemv8a.dtb foundation-v8.dtb
dtb-$(CONFIG_ARCH_XGENE) += apm-mustang.dtb

diff --git a/arch/arm64/boot/dts/sprd-sc9836-openphone.dts b/arch/arm64/boot/dts/sprd-sc9836-openphone.dts
new file mode 100644
index 0000000..484d714
--- /dev/null
+++ b/arch/arm64/boot/dts/sprd-sc9836-openphone.dts
@@ -0,0 +1,85 @@
+/*
+ * Spreadtrum SC9836 openphone board DTS file
+ *
+ * Copyright (C) 2014, Spreadtrum Communications Inc.
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ * a) This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * This library 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.
+ *
+ * Or, alternatively,
+ *
+ * b) Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+
+#include "sprd-sc9836.dtsi"
+
+/ {
+ model = "Spreadtrum,SC9836 Openphone Board";
+
+ compatible = "sprd,sc9836-openphone", "sprd,sc9836";
+
+ aliases {
+ serial0 = &uart0;
+ serial1 = &uart1;
+ serial2 = &uart2;
+ serial3 = &uart3;
+ };
+
+ memory@80000000 {
+ device_type = "memory";
+ reg = <0 0x80000000 0 0x20000000>;
+ };
+
+ chosen {
+ stdout-path = &uart0;
+ };
+};
+
+&uart0 {
+ status = "okay";
+};
+
+&uart1 {
+ status = "okay";
+};
+
+&uart2 {
+ status = "okay";
+};
+
+&uart3 {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/sprd-sc9836.dtsi b/arch/arm64/boot/dts/sprd-sc9836.dtsi
new file mode 100644
index 0000000..d5fe552
--- /dev/null
+++ b/arch/arm64/boot/dts/sprd-sc9836.dtsi
@@ -0,0 +1,103 @@
+/*
+ * Spreadtrum SC9836 SoC DTS file
+ *
+ * Copyright (C) 2014, Spreadtrum Communications Inc.
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ * a) This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * This library 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.
+ *
+ * Or, alternatively,
+ *
+ * b) Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include "sprd-sharkl64.dtsi"
+
+/ {
+ compatible = "sprd,sc9836";
+
+ cpus {
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ cpu@0 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53", "arm,armv8";
+ reg = <0x0 0x0>;
+ enable-method = "psci";
+ };
+ cpu@1 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53", "arm,armv8";
+ reg = <0x0 0x1>;
+ enable-method = "psci";
+ };
+ cpu@2 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53", "arm,armv8";
+ reg = <0x0 0x2>;
+ enable-method = "psci";
+ };
+ cpu@3 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53", "arm,armv8";
+ reg = <0x0 0x3>;
+ enable-method = "psci";
+ };
+ };
+
+ gic: interrupt-controller@12001000 {
+ compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";
+ #interrupt-cells = <3>;
+ interrupt-controller;
+ reg = <0 0x12001000 0 0x1000>,
+ <0 0x12002000 0 0x1000>,
+ <0 0x12004000 0 0x2000>,
+ <0 0x12006000 0 0x2000>;
+ };
+
+ psci {
+ compatible = "arm,psci-0.2";
+ method = "smc";
+ };
+
+ timer {
+ compatible = "arm,armv8-timer";
+ interrupts = <1 13 0xff01>,
+ <1 14 0xff01>,
+ <1 11 0xff01>,
+ <1 10 0xff01>;
+ clock-frequency = <26000000>;
+ };
+};
diff --git a/arch/arm64/boot/dts/sprd-sharkl64.dtsi b/arch/arm64/boot/dts/sprd-sharkl64.dtsi
new file mode 100644
index 0000000..d9ecfe9
--- /dev/null
+++ b/arch/arm64/boot/dts/sprd-sharkl64.dtsi
@@ -0,0 +1,105 @@
+/*
+ * Spreadtrum Sharkl64 platform DTS file
+ *
+ * Copyright (C) 2014, Spreadtrum Communications Inc.
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ * a) This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * This library 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.
+ *
+ * Or, alternatively,
+ *
+ * b) Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/ {
+ interrupt-parent = <&gic>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ soc {
+ compatible = "simple-bus";
+ reg = <0x0 0x0 0x0 0x80000000>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+ ap_apb: apb@70000000 {
+ compatible = "simple-bus";
+ reg = <0x0 0x70000000 0x0 0x10000000>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+ uart0: serial@70000000 {
+ compatible = "sprd,sc9836-uart";
+ reg = <0 0x70000000 0 0x100>;
+ interrupts = <0 2 0xf04>;
+ clocks = <&clk26mhz>;
+ status = "disabled";
+ };
+
+ uart1: serial@70100000 {
+ compatible = "sprd,sc9836-uart";
+ reg = <0 0x70100000 0 0x100>;
+ interrupts = <0 3 0xf04>;
+ clocks = <&clk26mhz>;
+ status = "disabled";
+ };
+
+ uart2: serial@70200000 {
+ compatible = "sprd,sc9836-uart";
+ reg = <0 0x70200000 0 0x100>;
+ interrupts = <0 2 0xf04>;
+ clocks = <&clk26mhz>;
+ status = "disabled";
+ };
+
+ uart3: serial@70300000 {
+ compatible = "sprd,sc9836-uart";
+ reg = <0 0x70300000 0 0x100>;
+ interrupts = <0 3 0xf04>;
+ clocks = <&clk26mhz>;
+ status = "disabled";
+ };
+ };
+ };
+
+ clocks {
+ clk26mhz: clk26mhz {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <26000000>;
+ };
+ };
+};
--
1.7.9.5

2014-11-25 20:03:58

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

On Tue, Nov 25, 2014 at 08:16:58PM +0800, 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.
>
> Signed-off-by: Chunyan Zhang <[email protected]>
> Signed-off-by: Orson Zhai <[email protected]>
> Originally-by: Lanqing Liu <[email protected]>

Some objections:

> +static struct platform_driver sprd_platform_driver = {
> + .probe = sprd_probe,
> + .remove = sprd_remove,
> + .suspend = sprd_suspend,
> + .resume = sprd_resume,
> + .driver = {
> + .name = "sprd_serial",
> + .owner = THIS_MODULE,

platform drivers don't need .owner in them anymore, it's handled by the
driver core automatically.

> + .of_match_table = of_match_ptr(serial_ids),
> + },
> +};
> +
> +static int __init sprd_serial_init(void)
> +{
> + int ret = 0;
> +
> + ret = uart_register_driver(&sprd_uart_driver);
> + if (unlikely(ret != 0))
> + return ret;

NEVER use unlikely unless you can measure the difference without it.
And even then, you better be able to justify it. For something as dumb
as this type of check, youare working against the complier and cpu which
already knows that 0 is the usual response.

So please remove all usages of likely/unlikely in this patch series.

> +
> + ret = platform_driver_register(&sprd_platform_driver);
> + if (unlikely(ret != 0))
> + uart_unregister_driver(&sprd_uart_driver);
> +
> + return ret;
> +}
> +
> +static void __exit sprd_serial_exit(void)
> +{
> + platform_driver_unregister(&sprd_platform_driver);
> + uart_unregister_driver(&sprd_uart_driver);
> +}
> +
> +module_init(sprd_serial_init);
> +module_exit(sprd_serial_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Spreadtrum SoC serial driver series");
> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
> index 16ad852..d9a8c88 100644
> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -247,4 +247,7 @@
> /* MESON */
> #define PORT_MESON 109
>
> +/* SPRD SERIAL */
> +#define PORT_SPRD 110

Please use a tab character here.

2014-11-26 03:08:07

[permalink] [raw]

Subject: Re: [PATCH v3 4/5] arm64: Add support for Spreadtrum's Sharkl64 Platform in Kconfig and defconfig

2014-11-25 20:57 GMT+08:00 Arnd Bergmann <[email protected]>:
>
> On Tuesday 25 November 2014 20:16:57 Chunyan Zhang wrote:
> >
> > +menuconfig ARCH_SPRD
> > + bool "Spreadtrum SoC platform"
> > + depends on ARM64
> > + help
> > + Support for Spreadtrum ARM based SoCs
> > +
> > +if ARCH_SPRD
> > +
> > +config ARCH_SHARKL64
> > + bool "Sharkl64 SoC Platform"
> > + help
> > + Sharkl64 is a Spreadtrum's SoC Platform which is based
> > + on ARM 64-bit processor core including
> > + sc9836
> > +
> > +endif #ARCH_SPRD
> > +
>
> I don't think we need multiple levels here, it should be enough to
> have either ARCH_SPRD or ARCH_SHARKL64, because all device drivers
> are going to be optional anyway. Typically a Kconfig symbol covers
> all SoCs that are related, so if you Spreadtrum are doing both
> phone and server chips and these are designed independently, you
> would have two symbols, but if you only expect to see phone chips
> here that are all derived from the same product line, using ARCH_SPRD
> to refer to all of them should be enough.
>
> Arnd

For now, we have only one platform(Sharkl64) based on ARM64 been
submitted, but we're intending to add support for more our platforms
based on ARM64 or ARM32 in the future. There are many common devices
on these platforms, such as serial. Our idea would be that if we had a
'menuconfig ARCH_SPRD' in the Kconfig, these common devices only need
to depend on ARCH_SPRD in the respective Kconfig, otherwise they may
depend on a few Kconfig symbols for every platforms which include
these common devices.

So, do you think whether we should define a menuconfig(ARCH_SPRD) in
the Kconfig for this case ?

Thanks!

Best regards,
Chunyan

2014-11-26 09:01:42

[permalink] [raw]

Subject: Re: [PATCH v3 4/5] arm64: Add support for Spreadtrum's Sharkl64 Platform in Kconfig and defconfig

On Wednesday 26 November 2014 10:32:35 Lyra Zhang wrote:
>
> For now, we have only one platform(Sharkl64) based on ARM64 been submitted,
> but we're intending to add support for more our platforms based on ARM64 or
> ARM32 in the future. There are many common devices on these platforms, such
> as serial. Our idea would be that if we had a 'menuconfig ARCH_SPRD' in the
> Kconfig, these common devices only need to depend on ARCH_SPRD in the
> respective Kconfig, otherwise they may depend on a few Kconfig symbols for
> every platforms which include these common devices.
>
> So, do you think whether we should define a menuconfig(ARCH_SPRD) in the
> Kconfig for this case ?

It sounds like your other platforms are all related, so one ARCH_SPRD
should be enough. If someone wants to build a kernel that is only
going to run on a particular machine, they can just turn off all the
other drivers they don't want.

Arnd

2014-11-26 09:48:46

by Tobias Klauser

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

On 2014-11-25 at 13:16:58 +0100, Chunyan Zhang <[email protected]> wrote:
> Add a full sc9836-uart driver for SC9836 SoC which is based on the
> spreadtrum sharkl64 platform.
> This driver also support earlycon.
>
> Signed-off-by: Chunyan Zhang <[email protected]>
> Signed-off-by: Orson Zhai <[email protected]>
> Originally-by: Lanqing Liu <[email protected]>
> ---
> Documentation/devices.txt | 3 +
> drivers/tty/serial/Kconfig | 23 ++
> drivers/tty/serial/Makefile | 1 +
> drivers/tty/serial/sprd_serial.c | 752 ++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/serial_core.h | 3 +
> 5 files changed, 782 insertions(+)
> create mode 100644 drivers/tty/serial/sprd_serial.c
>
[...]
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1573,6 +1573,29 @@ 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 SPRD serial"
> + depends on ARCH_SPRD
> + select SERIAL_CORE
> + help
> + This enables the driver for the Spreadtrum's serial.
> +
> +config SERIAL_SPRD_NR
> + int "Maximum number of sprd serial ports"
> + depends on SERIAL_SPRD
> + default "4"
> +
> +config SERIAL_SPRD_CONSOLE
> + bool "SPRD 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

Please consistently use tabs instead of spaces for indentation. The help
text should be indented by one tabe + 2 spaces.

[...]
> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> new file mode 100644
> index 0000000..58214c8
> --- /dev/null
> +++ b/drivers/tty/serial/sprd_serial.c
[...]
> +static inline int handle_lsr_errors(struct uart_port *port,
> + unsigned int *flag, unsigned int *lsr)

This line should be aligned with the opening ( above.

> +{
> + int ret = 0;
> +
> + /* stastics */
> + 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(int irq, void *dev_id)
> +{
> + struct uart_port *port = (struct uart_port *)dev_id;

No need to cast a void pointer.

> + struct tty_port *tty = &port->state->port;
> + unsigned int ch, flag, lsr, max_count = 2048;
> +
> + 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 (unlikely(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 void sprd_console_write(struct console *co, const char *s,
> + unsigned int count)
> +{
> + struct uart_port *port = (struct uart_port *)sprd_port[co->index];

Better explicitly access the .port member of sprd_port[co->index] here
instead of casting.

> + int ien;
> + int locked = 1;
> +
> + if (oops_in_progress)
> + locked = spin_trylock(&port->lock);
> + else
> + spin_lock(&port->lock);
> + /* save the IEN then disable the interrupts */
> + ien = serial_in(port, SPRD_IEN);
> + serial_out(port, SPRD_IEN, 0x0);
> +
> + uart_console_write(port, s, count, sprd_console_putchar);
> +
> + /* wait for transmitter to become empty and restore the IEN */
> + wait_for_xmitr(port);
> + serial_out(port, SPRD_IEN, ien);
> + if (locked)
> + spin_unlock(&port->lock);
> +}
> +
> +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 (unlikely(co->index >= UART_NR_MAX || co->index < 0))
> + co->index = 0;
> +
> + port = (struct uart_port *)sprd_port[co->index];

Same here, use the .port member of struct sprd_port[co->index].

> + if (port == NULL) {
> + pr_info("srial port %d not yet initialized\n", co->index);

Typo: should be serial instead of srial.

> + return -ENODEV;
> + }
> + if (options)
> + uart_parse_options(options, &baud, &parity, &bits, &flow);
> +
> + return uart_set_options(port, co, baud, parity, bits, flow);
> +}
[...]
> +static int sprd_probe(struct platform_device *pdev)
> +{
> + struct resource *mem;
> + struct device_node *np = pdev->dev.of_node;
> + struct uart_port *up;
> + struct clk *clk;
> + int irq;
> +
> +
> + if (np)
> + pdev->id = of_alias_get_id(np, "serial");
> +
> + if (unlikely(pdev->id < 0 || pdev->id >= UART_NR_MAX)) {
> + dev_err(&pdev->dev, "does not support id %d\n", pdev->id);
> + return -ENXIO;
> + }
> +
> + sprd_port[pdev->id] = devm_kzalloc(&pdev->dev,
> + sizeof(*sprd_port[pdev->id]), GFP_KERNEL);
> + if (!sprd_port[pdev->id])
> + return -ENOMEM;
> +
> + up = (struct uart_port *)sprd_port[pdev->id];
> + up->dev = &pdev->dev;
> + up->line = pdev->id;
> + 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 = ASYNC_BOOT_AUTOCONF;
> +
> + clk = devm_clk_get(&pdev->dev, NULL);
> + if (!IS_ERR(clk))
> + up->uartclk = clk_get_rate(clk);
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (unlikely(!mem)) {
> + dev_err(&pdev->dev, "not provide mem resource\n");
> + return -ENODEV;
> + }
> + up->mapbase = mem->start;
> + up->membase = ioremap(mem->start, resource_size(mem));

Return value of ioremap() should be checked for NULL.

> +
> + irq = platform_get_irq(pdev, 0);
> + if (unlikely(irq < 0)) {
> + dev_err(&pdev->dev, "not provide irq resource\n");
> + return -ENODEV;
> + }
> + up->irq = irq;
> +
> + platform_set_drvdata(pdev, up);
> +
> + return uart_add_one_port(&sprd_uart_driver, up);
> +}
> +
> +static int sprd_remove(struct platform_device *dev)
> +{
> + struct uart_port *up = platform_get_drvdata(dev);
> +
> + return uart_remove_one_port(&sprd_uart_driver, up);
> +}
> +
> +static int sprd_suspend(struct platform_device *dev, pm_message_t state)
> +{
> + int id = dev->id;
> + struct uart_port *port = (struct uart_port *)sprd_port[id];
> + struct reg_backup *reg_bak = &(sprd_port[id]->reg_bak);
> +
> + reg_bak->ien = serial_in(port, SPRD_IEN);
> + reg_bak->ctrl0 = serial_in(port, SPRD_LCR);
> + reg_bak->ctrl1 = serial_in(port, SPRD_CTL1);
> + reg_bak->ctrl2 = serial_in(port, SPRD_CTL2);
> + reg_bak->clkd0 = serial_in(port, SPRD_CLKD0);
> + reg_bak->clkd1 = serial_in(port, SPRD_CLKD1);
> +
> + return 0;
> +}
> +
> +static int sprd_resume(struct platform_device *dev)
> +{
> + int id = dev->id;
> + struct uart_port *port = (struct uart_port *)sprd_port[id];

Access the .port member instead of the cast.

> + struct reg_backup *reg_bak = &(sprd_port[id]->reg_bak);
> +
> + serial_out(port, SPRD_LCR, reg_bak->ctrl0);
> + serial_out(port, SPRD_CTL1, reg_bak->ctrl1);
> + serial_out(port, SPRD_CTL2, reg_bak->ctrl2);
> + serial_out(port, SPRD_CLKD0, reg_bak->clkd0);
> + serial_out(port, SPRD_CLKD1, reg_bak->clkd1);
> + serial_out(port, SPRD_IEN, reg_bak->ien);
> +
> + return 0;
> +}

2014-11-26 12:41:09

by Alan Cox

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

> + 213 = /dev/ttySPX0 SPRD serial port 0
> + ...
> + 216 = /dev/ttySPX3 SPRD serial port 3

Please use dynamic allocation or give a very very good reason why you
can't

> +config SERIAL_SPRD_NR
> + int "Maximum number of sprd serial ports"
> + depends on SERIAL_SPRD
> + default "4"

If you are doing dynamic allocation this should pretty much go away

> +static int sprd_startup(struct uart_port *port)
> +{
> + int ret = 0;
> + unsigned int ien, ctrl1;
> + struct sprd_uart_port *sp;
> +
> + serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY << 8) | THLD_RX_FULL));
> +
> + /* clear rx fifo */
> + while (serial_in(port, SPRD_STS1) & 0x00ff)
> + serial_in(port, SPRD_RXD);
> +
> + /* clear tx fifo */
> + while (serial_in(port, SPRD_STS1) & 0xff00)
> + ;

Missing a cpu_relax and I would have thought a timeout on both of these.

> +static void sprd_set_termios(struct uart_port *port,
> + struct ktermios *termios,
> + struct ktermios *old)
> +{
> + unsigned int baud, quot;

> + /* calculate parity */
> + lcr &= ~SPRD_LCR_PARITY;
> + 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;
> + }

If you don't support mark/space parity then also clear CMSPAR in
termios->c_cflag. If you do then it ought to be handled above.

> +
> + /* clock divider bit16~bit20 */
> + serial_out(port, SPRD_CLKD1, (quot & 0x1f0000) >> 16);
> + serial_out(port, SPRD_LCR, lcr);
> + fc |= 0x3e00 | THLD_RX_FULL;
> + serial_out(port, SPRD_CTL1, fc);

Also set the resulting baud back into the termios (see the 8250 termios
for an example)

> +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 (unlikely(co->index >= UART_NR_MAX || co->index < 0))
> + co->index = 0;
> +
> + port = (struct uart_port *)sprd_port[co->index];
> + if (port == NULL) {
> + pr_info("srial port %d not yet initialized\n", co->index);

Typo

Looks basically sound to me

Alan

2014-11-26 18:31:43

by Karicheri, Muralidharan

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

On 11/25/2014 07:16 AM, 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.
>
> Signed-off-by: Chunyan Zhang<[email protected]>
> Signed-off-by: Orson Zhai<[email protected]>
> Originally-by: Lanqing Liu<[email protected]>
> ---
> Documentation/devices.txt | 3 +
> drivers/tty/serial/Kconfig | 23 ++
> drivers/tty/serial/Makefile | 1 +
> drivers/tty/serial/sprd_serial.c | 752 ++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/serial_core.h | 3 +
> 5 files changed, 782 insertions(+)
> create mode 100644 drivers/tty/serial/sprd_serial.c
>
> diff --git a/Documentation/devices.txt b/Documentation/devices.txt
> index 87b4c5e..1da0432 100644
> --- a/Documentation/devices.txt
> +++ b/Documentation/devices.txt
> @@ -2816,6 +2816,9 @@ Your cooperation is appreciated.
> 210 = /dev/ttyMAX1 MAX3100 serial port 1
> 211 = /dev/ttyMAX2 MAX3100 serial port 2
> 212 = /dev/ttyMAX3 MAX3100 serial port 3
> + 213 = /dev/ttySPX0 SPRD serial port 0
> + ...
> + 216 = /dev/ttySPX3 SPRD serial port 3
>
> 205 char Low-density serial ports (alternate device)
> 0 = /dev/culu0 Callout device for ttyLU0
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 649b784..2c2cf60 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1573,6 +1573,29 @@ 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 SPRD serial"
> + depends on ARCH_SPRD
> + select SERIAL_CORE
> + help
> + This enables the driver for the Spreadtrum's serial.
> +
> +config SERIAL_SPRD_NR
> + int "Maximum number of sprd serial ports"
> + depends on SERIAL_SPRD
> + default "4"
> +
> +config SERIAL_SPRD_CONSOLE
> + bool "SPRD 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..58214c8
> --- /dev/null
> +++ b/drivers/tty/serial/sprd_serial.c
> @@ -0,0 +1,752 @@
> +/*
> + * Copyright (C) 2012 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.
> + */
> +
> +#include<linux/module.h>
> +#include<linux/tty.h>
> +#include<linux/ioport.h>
> +#include<linux/console.h>
> +#include<linux/platform_device.h>
> +#include<linux/tty_flip.h>
> +#include<linux/serial_core.h>
> +#include<linux/serial.h>
> +#include<linux/delay.h>
> +#include<linux/io.h>
> +#include<asm/irq.h>
> +#include<linux/slab.h>
> +#include<linux/of.h>
> +#include<linux/kernel.h>
> +#include<linux/slab.h>
> +#include<linux/clk.h>
How about sorting this includes? asm/irq.h go first followed linux/ in
alphabatical order?
> +
> +/* device name */
> +#define UART_NR_MAX CONFIG_SERIAL_SPRD_NR
> +#define SPRD_TTY_NAME "ttySPX"
> +#define SPRD_TTY_MAJOR 204
> +#define SPRD_TTY_MINOR_START 213
> +#define SPRD_FIFO_SIZE 128
> +#define SPRD_DEF_RATE 26000000
> +
> +/* 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)
> +
> +/* 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 {
> + uint32_t ien;
> + uint32_t ctrl0;
> + uint32_t ctrl1;
> + uint32_t ctrl2;
> + uint32_t clkd0;
> + uint32_t clkd1;
> + uint32_t 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] = { NULL };
> +
> +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 inline int handle_lsr_errors(struct uart_port *port,
> + unsigned int *flag, unsigned int *lsr)
> +{
> + int ret = 0;
> +
> + /* stastics */
> + 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(int irq, void *dev_id)
> +{
> + struct uart_port *port = (struct uart_port *)dev_id;
> + struct tty_port *tty =&port->state->port;
> + unsigned int ch, flag, lsr, max_count = 2048;
> +
> + 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 (unlikely(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(int irq, void *dev_id)
> +{
> + struct uart_port *port = dev_id;
> + 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 = (struct uart_port *)dev_id;
> + u32 ims;
> +
> + ims = serial_in(port, SPRD_IMSR);
> +
> + serial_out(port, SPRD_ICLR, ~0);
> +
> + if (ims& (SPRD_IMSR_RX_FIFO_FULL |
> + SPRD_IMSR_BREAK_DETECT | SPRD_IMSR_TIMEOUT)) {
> + sprd_rx(irq, port);
> + }
> + if (ims& SPRD_IMSR_TX_FIFO_EMPTY)
> + sprd_tx(irq, port);
> +
> + return IRQ_HANDLED;
You are always returning IRQ_HANDLED and this is registered as a SHARED
irq. Is there a chance this handler is called and the irq event doesn't
belong to this device?

Murali
> +}
> +
> +static int sprd_startup(struct uart_port *port)
> +{
> + int ret = 0;
> + unsigned int ien, ctrl1;
> + struct sprd_uart_port *sp;
> +
> + serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY<< 8) | THLD_RX_FULL));
> +
> + /* clear rx fifo */
> + while (serial_in(port, SPRD_STS1)& 0x00ff)
> + serial_in(port, SPRD_RXD);
> +
> + /* clear tx fifo */
> + while (serial_in(port, SPRD_STS1)& 0xff00)
> + ;
> +
> + /* clear interrupt */
> + serial_out(port, SPRD_IEN, 0x0);
> + 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\n",
> + port->irq);
> + return ret;
> + }
> + ctrl1 = serial_in(port, SPRD_CTL1);
> + ctrl1 |= 0x3e00 | THLD_RX_FULL;
> + serial_out(port, SPRD_CTL1, ctrl1);
> +
> + /* enable interrupt */
> + spin_lock(&port->lock);
> + 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(&port->lock);
> +
> + return 0;
> +}
> +
> +static void sprd_shutdown(struct uart_port *port)
> +{
> + serial_out(port, SPRD_IEN, 0x0);
> + 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, fc;
> +
> + /* ask the core to calculate the divisor for us */
> + baud = uart_get_baud_rate(port, termios, old, 1200, 3000000);
> +
> + quot = (unsigned int)((port->uartclk + baud / 2) / baud);
> +
> + /* set data length */
> + lcr = serial_in(port, SPRD_LCR);
> + lcr&= ~SPRD_LCR_DATA_LEN;
> + 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;
> + 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;
> + }
> +
> + /* change the port state. */
> + /* 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& (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 |= 0x3e00 | THLD_RX_FULL;
> + serial_out(port, SPRD_CTL1, fc);
> +}
> +
> +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 (unlikely(ser->type != PORT_SPRD))
> + return -EINVAL;
> + if (unlikely(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 = (struct uart_port *)sprd_port[co->index];
> + int ien;
> + int locked = 1;
> +
> + if (oops_in_progress)
> + locked = spin_trylock(&port->lock);
> + else
> + spin_lock(&port->lock);
> + /* save the IEN then disable the interrupts */
> + ien = serial_in(port, SPRD_IEN);
> + serial_out(port, SPRD_IEN, 0x0);
> +
> + uart_console_write(port, s, count, sprd_console_putchar);
> +
> + /* wait for transmitter to become empty and restore the IEN */
> + wait_for_xmitr(port);
> + serial_out(port, SPRD_IEN, ien);
> + if (locked)
> + spin_unlock(&port->lock);
> +}
> +
> +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 (unlikely(co->index>= UART_NR_MAX || co->index< 0))
> + co->index = 0;
> +
> + port = (struct uart_port *)sprd_port[co->index];
> + if (port == NULL) {
> + pr_info("srial 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)
> +{
> + while (!(readl(port->membase + SPRD_LSR)& SPRD_LSR_TX_OVER))
> + ;
> + 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 = SPRD_TTY_MAJOR,
> + .minor = SPRD_TTY_MINOR_START,
> + .nr = UART_NR_MAX,
> + .cons = SPRD_CONSOLE,
> +};
> +
> +static int sprd_probe(struct platform_device *pdev)
> +{
> + struct resource *mem;
> + struct device_node *np = pdev->dev.of_node;
> + struct uart_port *up;
> + struct clk *clk;
> + int irq;
> +
> +
> + if (np)
> + pdev->id = of_alias_get_id(np, "serial");
> +
> + if (unlikely(pdev->id< 0 || pdev->id>= UART_NR_MAX)) {
> + dev_err(&pdev->dev, "does not support id %d\n", pdev->id);
> + return -ENXIO;
> + }
> +
> + sprd_port[pdev->id] = devm_kzalloc(&pdev->dev,
> + sizeof(*sprd_port[pdev->id]), GFP_KERNEL);
> + if (!sprd_port[pdev->id])
> + return -ENOMEM;
> +
> + up = (struct uart_port *)sprd_port[pdev->id];
> + up->dev =&pdev->dev;
> + up->line = pdev->id;
> + 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 = ASYNC_BOOT_AUTOCONF;
> +
> + clk = devm_clk_get(&pdev->dev, NULL);
> + if (!IS_ERR(clk))
> + up->uartclk = clk_get_rate(clk);
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (unlikely(!mem)) {
> + dev_err(&pdev->dev, "not provide mem resource\n");
> + return -ENODEV;
> + }
> + up->mapbase = mem->start;
> + up->membase = ioremap(mem->start, resource_size(mem));
> +
> + irq = platform_get_irq(pdev, 0);
> + if (unlikely(irq< 0)) {
> + dev_err(&pdev->dev, "not provide irq resource\n");
> + return -ENODEV;
> + }
> + up->irq = irq;
> +
> + platform_set_drvdata(pdev, up);
> +
> + return uart_add_one_port(&sprd_uart_driver, up);
> +}
> +
> +static int sprd_remove(struct platform_device *dev)
> +{
> + struct uart_port *up = platform_get_drvdata(dev);
> +
> + return uart_remove_one_port(&sprd_uart_driver, up);
> +}
> +
> +static int sprd_suspend(struct platform_device *dev, pm_message_t state)
> +{
> + int id = dev->id;
> + struct uart_port *port = (struct uart_port *)sprd_port[id];
> + struct reg_backup *reg_bak =&(sprd_port[id]->reg_bak);
> +
> + reg_bak->ien = serial_in(port, SPRD_IEN);
> + reg_bak->ctrl0 = serial_in(port, SPRD_LCR);
> + reg_bak->ctrl1 = serial_in(port, SPRD_CTL1);
> + reg_bak->ctrl2 = serial_in(port, SPRD_CTL2);
> + reg_bak->clkd0 = serial_in(port, SPRD_CLKD0);
> + reg_bak->clkd1 = serial_in(port, SPRD_CLKD1);
> +
> + return 0;
> +}
> +
> +static int sprd_resume(struct platform_device *dev)
> +{
> + int id = dev->id;
> + struct uart_port *port = (struct uart_port *)sprd_port[id];
> + struct reg_backup *reg_bak =&(sprd_port[id]->reg_bak);
> +
> + serial_out(port, SPRD_LCR, reg_bak->ctrl0);
> + serial_out(port, SPRD_CTL1, reg_bak->ctrl1);
> + serial_out(port, SPRD_CTL2, reg_bak->ctrl2);
> + serial_out(port, SPRD_CLKD0, reg_bak->clkd0);
> + serial_out(port, SPRD_CLKD1, reg_bak->clkd1);
> + serial_out(port, SPRD_IEN, reg_bak->ien);
> +
> + return 0;
> +}
> +
> +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,
> + .suspend = sprd_suspend,
> + .resume = sprd_resume,
> + .driver = {
> + .name = "sprd_serial",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(serial_ids),
> + },
> +};
> +
> +static int __init sprd_serial_init(void)
> +{
> + int ret = 0;
> +
> + ret = uart_register_driver(&sprd_uart_driver);
> + if (unlikely(ret != 0))
> + return ret;
> +
> + ret = platform_driver_register(&sprd_platform_driver);
> + if (unlikely(ret != 0))
> + uart_unregister_driver(&sprd_uart_driver);
> +
> + return ret;
> +}
> +
> +static void __exit sprd_serial_exit(void)
> +{
> + platform_driver_unregister(&sprd_platform_driver);
> + uart_unregister_driver(&sprd_uart_driver);
> +}
> +
> +module_init(sprd_serial_init);
> +module_exit(sprd_serial_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Spreadtrum SoC serial driver series");
> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
> index 16ad852..d9a8c88 100644
> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -247,4 +247,7 @@
> /* MESON */
> #define PORT_MESON 109
>
> +/* SPRD SERIAL */
> +#define PORT_SPRD 110
> +
> #endif /* _UAPILINUX_SERIAL_CORE_H */

--
Murali Karicheri
Linux Kernel, Texas Instruments

2014-11-27 11:05:13

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

2014-11-26 4:03 GMT+08:00 Greg KH <[email protected]>:
> On Tue, Nov 25, 2014 at 08:16:58PM +0800, 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.
>>
>> Signed-off-by: Chunyan Zhang <[email protected]>
>> Signed-off-by: Orson Zhai <[email protected]>
>> Originally-by: Lanqing Liu <[email protected]>
>
> Some objections:
>
>> +static struct platform_driver sprd_platform_driver = {
>> + .probe = sprd_probe,
>> + .remove = sprd_remove,
>> + .suspend = sprd_suspend,
>> + .resume = sprd_resume,
>> + .driver = {
>> + .name = "sprd_serial",
>> + .owner = THIS_MODULE,
>
> platform drivers don't need .owner in them anymore, it's handled by the
> driver core automatically.
>
OK, will remove it in v4.
>
>> + .of_match_table = of_match_ptr(serial_ids),
>> + },
>> +};
>> +
>> +static int __init sprd_serial_init(void)
>> +{
>> + int ret = 0;
>> +
>> + ret = uart_register_driver(&sprd_uart_driver);
>> + if (unlikely(ret != 0))
>> + return ret;
>
> NEVER use unlikely unless you can measure the difference without it.
> And even then, you better be able to justify it. For something as dumb
> as this type of check, youare working against the complier and cpu which
> already knows that 0 is the usual response.
>
> So please remove all usages of likely/unlikely in this patch series.
>
OK, will remove it.
>> +
>> + ret = platform_driver_register(&sprd_platform_driver);
>> + if (unlikely(ret != 0))
>> + uart_unregister_driver(&sprd_uart_driver);
>> +
>> + return ret;
>> +}
>> +
>> +static void __exit sprd_serial_exit(void)
>> +{
>> + platform_driver_unregister(&sprd_platform_driver);
>> + uart_unregister_driver(&sprd_uart_driver);
>> +}
>> +
>> +module_init(sprd_serial_init);
>> +module_exit(sprd_serial_exit);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Spreadtrum SoC serial driver series");
>> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
>> index 16ad852..d9a8c88 100644
>> --- a/include/uapi/linux/serial_core.h
>> +++ b/include/uapi/linux/serial_core.h
>> @@ -247,4 +247,7 @@
>> /* MESON */
>> #define PORT_MESON 109
>>
>> +/* SPRD SERIAL */
>> +#define PORT_SPRD 110
>
> Please use a tab character here.
OK.

Thanks for your comments!
Chunyan

2014-11-27 11:39:30

[permalink] [raw]

Subject: Re: [PATCH v3 1/5] Documentation: DT: Renamed of-serial.txt to 8250.txt

On Tue, Nov 25, 2014 at 12:16:54PM +0000, Chunyan Zhang wrote:
> The file of-serial.txt was only for 8250 compatible UART implementations,
> so renamed it to 8250.txt to avoid confusing other persons.
> This is recommended by Arnd, see:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/291455.html
>
> Signed-off-by: Chunyan Zhang <[email protected]>

My prior Ack [1] still applies (given you've just changed the way the
patch was generated, as Arnd and I suggested).

Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/295152.html

> ---
> .../bindings/serial/{of-serial.txt => 8250.txt} | 0
> 1 file changed, 0 insertions(+), 0 deletions(-)
> rename Documentation/devicetree/bindings/serial/{of-serial.txt => 8250.txt} (100%)
>
> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/8250.txt
> similarity index 100%
> rename from Documentation/devicetree/bindings/serial/of-serial.txt
> rename to Documentation/devicetree/bindings/serial/8250.txt
> --
> 1.7.9.5
>
>

2014-11-27 11:39:41

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

2014-11-26 17:48 GMT+08:00 Tobias Klauser <[email protected]>:
> On 2014-11-25 at 13:16:58 +0100, Chunyan Zhang <[email protected]> wrote:

>> ---
[...]
>> +
>> +config SERIAL_SPRD_CONSOLE
>> + bool "SPRD 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
>
> Please consistently use tabs instead of spaces for indentation. The help
> text should be indented by one tabe + 2 spaces.
>

OK, I will note it later.
[...]

>> +static inline int handle_lsr_errors(struct uart_port *port,
>> + unsigned int *flag, unsigned int *lsr)
>
> This line should be aligned with the opening ( above.
>
OK, will change.

>> +static inline void sprd_rx(int irq, void *dev_id)
>> +{
>> + struct uart_port *port = (struct uart_port *)dev_id;
>
> No need to cast a void pointer.
>

>> +static void sprd_console_write(struct console *co, const char *s,
>> + unsigned int count)
>> +{
>> + struct uart_port *port = (struct uart_port *)sprd_port[co->index];
>
> Better explicitly access the .port member of sprd_port[co->index] here
> instead of casting.
>

>> + port = (struct uart_port *)sprd_port[co->index];
>
> Same here, use the .port member of struct sprd_port[co->index].
>
>> + if (port == NULL) {
>> + pr_info("srial port %d not yet initialized\n", co->index);
>
> Typo: should be serial instead of srial.
>

>> + up->mapbase = mem->start;
>> + up->membase = ioremap(mem->start, resource_size(mem));
>
> Return value of ioremap() should be checked for NULL.
>
>> +static int sprd_resume(struct platform_device *dev)
>> +{
>> + int id = dev->id;
>> + struct uart_port *port = (struct uart_port *)sprd_port[id];
>
> Access the .port member instead of the cast.
>
OK, will change all of the problems you pointed out in v4.
Thanks for your review.

Chunyan

2014-11-27 11:51:40

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

On Tue, Nov 25, 2014 at 12:16:56PM +0000, Chunyan Zhang wrote:
> From: Zhizhou Zhang <[email protected]>
>
> Adds the device tree support for Spreadtrum SC9836 SoC which is based on
> Sharkl64 platform.
>
> Sharkl64 platform contains the common nodes of Spreadtrum's arm64-based SoCs.
>
> Signed-off-by: Zhizhou Zhang <[email protected]>
> Signed-off-by: Chunyan Zhang <[email protected]>
> Signed-off-by: Orson Zhai <[email protected]>
> ---
> arch/arm64/boot/dts/Makefile | 1 +
> arch/arm64/boot/dts/sprd-sc9836-openphone.dts | 85 ++++++++++++++++++++
> arch/arm64/boot/dts/sprd-sc9836.dtsi | 103 ++++++++++++++++++++++++
> arch/arm64/boot/dts/sprd-sharkl64.dtsi | 105 +++++++++++++++++++++++++
> 4 files changed, 294 insertions(+)
> create mode 100644 arch/arm64/boot/dts/sprd-sc9836-openphone.dts
> create mode 100644 arch/arm64/boot/dts/sprd-sc9836.dtsi
> create mode 100644 arch/arm64/boot/dts/sprd-sharkl64.dtsi
>
> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
> index f8001a6..d0aff8a 100644
> --- a/arch/arm64/boot/dts/Makefile
> +++ b/arch/arm64/boot/dts/Makefile
> @@ -1,4 +1,5 @@
> dtb-$(CONFIG_ARCH_THUNDER) += thunder-88xx.dtb
> +dtb-$(CONFIG_ARCH_SHARKL64) += sprd-sc9836-openphone.dtb
> dtb-$(CONFIG_ARCH_VEXPRESS) += rtsm_ve-aemv8a.dtb foundation-v8.dtb
> dtb-$(CONFIG_ARCH_XGENE) += apm-mustang.dtb

[...]

> + gic: interrupt-controller@12001000 {
> + compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";
> + #interrupt-cells = <3>;
> + interrupt-controller;
> + reg = <0 0x12001000 0 0x1000>,
> + <0 0x12002000 0 0x1000>,
> + <0 0x12004000 0 0x2000>,
> + <0 0x12006000 0 0x2000>;
> + };

I believe GICC should be 8KiB here.

> +
> + psci {
> + compatible = "arm,psci-0.2";
> + method = "smc";
> + };

Do you have a complete PSCI 0.2 implementation (e.g. are all the
mandatory functions implemented)?

I take it CPUs enter the kernel at EL2?

How have you tested this?

> +
> + timer {
> + compatible = "arm,armv8-timer";
> + interrupts = <1 13 0xff01>,
> + <1 14 0xff01>,
> + <1 11 0xff01>,
> + <1 10 0xff01>;
> + clock-frequency = <26000000>;

Please remove the clock-frequency property. Your FW should initialise
CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).

[...]

> + clocks {
> + clk26mhz: clk26mhz {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <26000000>;
> + };
> + };

Get rid of the "clocks" container node. There's no need for it.

Thanks,
Mark.

2014-11-27 11:59:52

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

2014-11-27 2:29 GMT+08:00 Murali Karicheri <[email protected]>:
> On 11/25/2014 07:16 AM, Chunyan Zhang wrote:
>>
>> Add a full sc9836-uart driver for SC9836 SoC which is based on the

>> +#include<linux/clk.h>
>
> How about sorting this includes? asm/irq.h go first followed linux/ in
> alphabatical order?

>> +static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
>> +{
>> + struct uart_port *port = (struct uart_port *)dev_id;
>> + u32 ims;
>> +
>> + ims = serial_in(port, SPRD_IMSR);
>> +
>> + serial_out(port, SPRD_ICLR, ~0);
>> +
>> + if (ims& (SPRD_IMSR_RX_FIFO_FULL |
>> + SPRD_IMSR_BREAK_DETECT | SPRD_IMSR_TIMEOUT)) {
>> + sprd_rx(irq, port);
>> + }
>> + if (ims& SPRD_IMSR_TX_FIFO_EMPTY)
>> + sprd_tx(irq, port);
>> +
>> + return IRQ_HANDLED;
>
> You are always returning IRQ_HANDLED and this is registered as a SHARED irq.
> Is there a chance this handler is called and the irq event doesn't
> belong to this device?
>
> Murali

You are right, this is not a SHARED irq. I'll pass 0 for irqflags when
called 'devm_request_irq' in the next version patch.

Thank you,
Chunyan

2014-11-27 12:04:54

[permalink] [raw]

Subject: Re: [PATCH v3 0/5] Add Spreadtrum Sharkl64 Platform support

Hi,

On Tue, Nov 25, 2014 at 12:16:53PM +0000, Chunyan Zhang wrote:
> Spreadtrum is a rapid growing chip vendor providing smart phone total solutions.
>
> Sharkl64 Platform is nominated as a SoC infrastructure that supports 4G/3G/2G
> standards based on ARMv8 multiple core architecture.Now we have only one
> SoC(SC9836) based on this Platform in developing.
>
> This patchset adds Sharkl64 support in arm64 device tree and the serial driver
> of SC9836-UART.
>
> This patchset also has patches which address "sprd" prefix and DT compatible
> strings for nodes which appear un-documented.
>
> This version code was tesed on Fast Mode.

This concerns me a bit because fast models are extremely lenient w.r.t.
the management of clocks and regulators. Are you sure you have
everything you need for your UART described?

> We use boot-wrapper-aarch64 as the bootloader.

Which version of boot-wrapper-aarch64 are you using?

Upstream does not implement PSCI 0.2, but its --enable-psci option will
inject PSCI 0.1 compatible properties, which leads me to suspect that
your DT is misleading. The boot wrapper should also program CNTFRQ_EL0
correctly regardless.

What do you intend to use on real hardware?

Thanks,
Mark.

>
> Changes from v2:
> * Addressed review comments:
> - Added a specific compitible string 'sc9836-uart' for the serial
> - Added a full serial driver
> - Added the property 'clock-frequency' for timer node in dtsi.
> - Replaceed the old macro prefix 'UART_' with 'SPRD_' in the
> Spreadtrum serial driver code.
> * Revised the name of SoC and board from 'sharkl3' to 'sc9836'
> * Used dual-license for DTS files
> * Added a menuconfig 'ARCH_SPRD' in arch/arm64/Kconfig
>
> Changes from v1:
> * Addressed review comments:
> - Added "sprd" prefix to vendor-prefixes.txt
> - Created serial/sprd-serial.txt and remove the properties for serial-sprd
> from of-serial.txt to it.
> - Renamed of-serial.txt to 8250.txt according to Arnd's review comments
> - Splited and revised .dts for Sharkl64 Platform
> - Changed to PSCI method for cpu power management
> - Revised Kconfig Makefile to match the alphabetical ordering
> - Renamed serial-sprd-earlycon.c to serial-sprd.c
>
> Chunyan Zhang (3):
> Documentation: DT: Renamed of-serial.txt to 8250.txt
> Documentation: DT: Add bindings for Spreadtrum SoC Platform
> tty/serial: Add Spreadtrum sc9836-uart driver support
>
> Zhizhou Zhang (2):
> arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile
> arm64: Add support for Spreadtrum's Sharkl64 Platform in Kconfig and
> defconfig
>
> Documentation/devices.txt | 3 +
> Documentation/devicetree/bindings/arm/sprd.txt | 11 +
> .../bindings/serial/{of-serial.txt => 8250.txt} | 0
> .../devicetree/bindings/serial/sprd-uart.txt | 6 +
> .../devicetree/bindings/vendor-prefixes.txt | 1 +
> arch/arm64/Kconfig | 17 +
> arch/arm64/boot/dts/Makefile | 1 +
> arch/arm64/boot/dts/sprd-sc9836-openphone.dts | 85 +++
> arch/arm64/boot/dts/sprd-sc9836.dtsi | 103 +++
> arch/arm64/boot/dts/sprd-sharkl64.dtsi | 105 +++
> arch/arm64/configs/defconfig | 2 +
> drivers/tty/serial/Kconfig | 23 +
> drivers/tty/serial/Makefile | 1 +
> drivers/tty/serial/sprd_serial.c | 752 ++++++++++++++++++++
> include/uapi/linux/serial_core.h | 3 +
> 15 files changed, 1113 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/sprd.txt
> rename Documentation/devicetree/bindings/serial/{of-serial.txt => 8250.txt} (100%)
> create mode 100644 Documentation/devicetree/bindings/serial/sprd-uart.txt
> create mode 100644 arch/arm64/boot/dts/sprd-sc9836-openphone.dts
> create mode 100644 arch/arm64/boot/dts/sprd-sc9836.dtsi
> create mode 100644 arch/arm64/boot/dts/sprd-sharkl64.dtsi
> create mode 100644 drivers/tty/serial/sprd_serial.c
>
> --
> 1.7.9.5
>
>

2014-11-27 12:09:01

[permalink] [raw]

Subject: Re: [PATCH v3 1/5] Documentation: DT: Renamed of-serial.txt to 8250.txt

2014-11-27 19:38 GMT+08:00 Mark Rutland <[email protected]>:
> On Tue, Nov 25, 2014 at 12:16:54PM +0000, Chunyan Zhang wrote:
>> The file of-serial.txt was only for 8250 compatible UART implementations,
>> so renamed it to 8250.txt to avoid confusing other persons.
>> This is recommended by Arnd, see:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/291455.html
>>
>> Signed-off-by: Chunyan Zhang <[email protected]>
>
> My prior Ack [1] still applies (given you've just changed the way the
> patch was generated, as Arnd and I suggested).
>
OK, I'll add in v4.

> Mark.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/295152.html
>
>> ---
>> .../bindings/serial/{of-serial.txt => 8250.txt} | 0
>> 1 file changed, 0 insertions(+), 0 deletions(-)
>> rename Documentation/devicetree/bindings/serial/{of-serial.txt => 8250.txt} (100%)
>>
>> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/8250.txt
>> similarity index 100%
>> rename from Documentation/devicetree/bindings/serial/of-serial.txt
>> rename to Documentation/devicetree/bindings/serial/8250.txt
>> --
>> 1.7.9.5
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2014-11-27 12:12:32

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

On Thu, Nov 27, 2014 at 11:50:43AM +0000, Mark Rutland wrote:
> On Tue, Nov 25, 2014 at 12:16:56PM +0000, Chunyan Zhang wrote:
> > +
> > + timer {
> > + compatible = "arm,armv8-timer";
> > + interrupts = <1 13 0xff01>,
> > + <1 14 0xff01>,
> > + <1 11 0xff01>,
> > + <1 10 0xff01>;
> > + clock-frequency = <26000000>;
>
> Please remove the clock-frequency property. Your FW should initialise
> CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).

Since this comes up regularly, I think we need a dev_warn() in the arch
timer driver when CONFIG_ARM64.

--
Catalin

2014-11-27 12:58:38

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

On Thursday 27 November 2014 19:59:46 Lyra Zhang wrote:
> 2014-11-27 2:29 GMT+08:00 Murali Karicheri <[email protected]>:
> > On 11/25/2014 07:16 AM, Chunyan Zhang wrote:
> >>
> >> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>
> >> +#include<linux/clk.h>
> >
> > How about sorting this includes? asm/irq.h go first followed linux/ in
> > alphabatical order?
>
> >> +static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
> >> +{
> >> + struct uart_port *port = (struct uart_port *)dev_id;
> >> + u32 ims;
> >> +
> >> + ims = serial_in(port, SPRD_IMSR);
> >> +
> >> + serial_out(port, SPRD_ICLR, ~0);
> >> +
> >> + if (ims& (SPRD_IMSR_RX_FIFO_FULL |
> >> + SPRD_IMSR_BREAK_DETECT | SPRD_IMSR_TIMEOUT)) {
> >> + sprd_rx(irq, port);
> >> + }
> >> + if (ims& SPRD_IMSR_TX_FIFO_EMPTY)
> >> + sprd_tx(irq, port);
> >> +
> >> + return IRQ_HANDLED;
> >
> > You are always returning IRQ_HANDLED and this is registered as a SHARED irq.
> > Is there a chance this handler is called and the irq event doesn't
> > belong to this device?
> >
> > Murali
>
> You are right, this is not a SHARED irq. I'll pass 0 for irqflags when
> called 'devm_request_irq' in the next version patch.

I think you could also add

if (!ims)
return IRQ_NONE;

which would make it work on shared interrupt lines.

Arnd

2014-11-27 13:44:03

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

On Thu, Nov 27, 2014 at 12:12:15PM +0000, Catalin Marinas wrote:
> On Thu, Nov 27, 2014 at 11:50:43AM +0000, Mark Rutland wrote:
> > On Tue, Nov 25, 2014 at 12:16:56PM +0000, Chunyan Zhang wrote:
> > > +
> > > + timer {
> > > + compatible = "arm,armv8-timer";
> > > + interrupts = <1 13 0xff01>,
> > > + <1 14 0xff01>,
> > > + <1 11 0xff01>,
> > > + <1 10 0xff01>;
> > > + clock-frequency = <26000000>;
> >
> > Please remove the clock-frequency property. Your FW should initialise
> > CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).
>
> Since this comes up regularly, I think we need a dev_warn() in the arch
> timer driver when CONFIG_ARM64.

I'll ack such a patch ;)

Mark.

2014-11-27 15:23:25

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

2014-11-27 20:57 GMT+08:00 Arnd Bergmann <[email protected]>:
> On Thursday 27 November 2014 19:59:46 Lyra Zhang wrote:
>> 2014-11-27 2:29 GMT+08:00 Murali Karicheri <[email protected]>:
>> > On 11/25/2014 07:16 AM, Chunyan Zhang wrote:
>> >>
>> >> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>>
>> >> +#include<linux/clk.h>
>> >
>> > How about sorting this includes? asm/irq.h go first followed linux/ in
>> > alphabatical order?
>>
>> >> +static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
>> >> +{
>> >> + struct uart_port *port = (struct uart_port *)dev_id;
>> >> + u32 ims;
>> >> +
>> >> + ims = serial_in(port, SPRD_IMSR);
>> >> +
>> >> + serial_out(port, SPRD_ICLR, ~0);
>> >> +
>> >> + if (ims& (SPRD_IMSR_RX_FIFO_FULL |
>> >> + SPRD_IMSR_BREAK_DETECT | SPRD_IMSR_TIMEOUT)) {
>> >> + sprd_rx(irq, port);
>> >> + }
>> >> + if (ims& SPRD_IMSR_TX_FIFO_EMPTY)
>> >> + sprd_tx(irq, port);
>> >> +
>> >> + return IRQ_HANDLED;
>> >
>> > You are always returning IRQ_HANDLED and this is registered as a SHARED irq.
>> > Is there a chance this handler is called and the irq event doesn't
>> > belong to this device?
>> >
>> > Murali
>>
>> You are right, this is not a SHARED irq. I'll pass 0 for irqflags when
>> called 'devm_request_irq' in the next version patch.
>
> I think you could also add
>
> if (!ims)
> return IRQ_NONE;
>
> which would make it work on shared interrupt lines.
>
> Arnd

Yes, I saw this way in other serial drivers.
But, if then, there are two questions for me:
1. Why did some serial drivers need an UN_SHARED irq?
2. How can we choose a right way?

Thank you!

Best regards,
Chunyan

2014-11-27 15:38:48

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

On Thursday 27 November 2014 23:23:17 Lyra Zhang wrote:
>
> Yes, I saw this way in other serial drivers.
> But, if then, there are two questions for me:
> 1. Why did some serial drivers need an UN_SHARED irq?

A lot of drivers were written before we had shared IRQs, or
were copied from old drivers, or are for hardware that uses
edge-triggered interrupts instead of level-triggered interrupts.

Only level-triggered interrupts can be shared.

> 2. How can we choose a right way?

It never hurts to implement a shared interrupt handler. Only
if you have no way to find out whether the device triggered
the interrupt or not you have to leave out IRQF_SHARED, and then
you won't be able to return IRQ_NONE.

Arnd

2014-11-27 15:39:17

by Alan Cox

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

> Yes, I saw this way in other serial drivers.
> But, if then, there are two questions for me:
> 1. Why did some serial drivers need an UN_SHARED irq?
> 2. How can we choose a right way?

If your hardware can support sharing an IRQ then shared is the right way
- along with the test Arnd suggested. Some hardware lacks the needed
facilities to handle shared interrupt.

Alan

2014-11-28 11:40:42

by Orson Zhai

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

Hi, Alan,

Thanks for your comments!
Some question for them below,

On 2014年11月26日 20:33, One Thousand Gnomes wrote:
>> + 213 = /dev/ttySPX0 SPRD serial port 0
>> + ...
>> + 216 = /dev/ttySPX3 SPRD serial port 3
> Please use dynamic allocation or give a very very good reason why you
> can't
do we just leave the .major & .minor as NULL in struct uart_driver for
dynamic allocation?
>> +config SERIAL_SPRD_NR
>> + int "Maximum number of sprd serial ports"
>> + depends on SERIAL_SPRD
>> + default "4"
> If you are doing dynamic allocation this should pretty much go away
I think you mean getting the number of uarts from dt and dynamically
allocate their port structure?
>
>> +static int sprd_startup(struct uart_port *port)
>> +{
>> + int ret = 0;
>> + unsigned int ien, ctrl1;
>> + struct sprd_uart_port *sp;
>> +
>> + serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY << 8) | THLD_RX_FULL));
>> +
>> + /* clear rx fifo */
>> + while (serial_in(port, SPRD_STS1) & 0x00ff)
>> + serial_in(port, SPRD_RXD);
>> +
>> + /* clear tx fifo */
>> + while (serial_in(port, SPRD_STS1) & 0xff00)
>> + ;
> Missing a cpu_relax and I would have thought a timeout on both of these.
We will add in V4
>
>
>> +static void sprd_set_termios(struct uart_port *port,
>> + struct ktermios *termios,
>> + struct ktermios *old)
>> +{
>> + unsigned int baud, quot;
>> + /* calculate parity */
>> + lcr &= ~SPRD_LCR_PARITY;
>> + 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;
>> + }
> If you don't support mark/space parity then also clear CMSPAR in
> termios->c_cflag.
just simply use code like "termios->c_cflag &= ~CMSPAR" ?

> If you do then it ought to be handled above.
>
>> +
>> + /* clock divider bit16~bit20 */
>> + serial_out(port, SPRD_CLKD1, (quot & 0x1f0000) >> 16);
>> + serial_out(port, SPRD_LCR, lcr);
>> + fc |= 0x3e00 | THLD_RX_FULL;
>> + serial_out(port, SPRD_CTL1, fc);
> Also set the resulting baud back into the termios (see the 8250 termios
> for an example)
you mean something like this part ?

/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios))
tty_termios_encode_baud_rate(termios, baud, baud);

>
>
>> +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 (unlikely(co->index >= UART_NR_MAX || co->index < 0))
>> + co->index = 0;
>> +
>> + port = (struct uart_port *)sprd_port[co->index];
>> + if (port == NULL) {
>> + pr_info("srial port %d not yet initialized\n", co->index);
> Typo
it will be fixed :)

Thanks,
Orson
> Looks basically sound to me
>
> Alan

2014-11-28 14:29:44

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

On Thu, Nov 27, 2014 at 01:43:09PM +0000, Mark Rutland wrote:
> On Thu, Nov 27, 2014 at 12:12:15PM +0000, Catalin Marinas wrote:
> > On Thu, Nov 27, 2014 at 11:50:43AM +0000, Mark Rutland wrote:
> > > On Tue, Nov 25, 2014 at 12:16:56PM +0000, Chunyan Zhang wrote:
> > > > +
> > > > + timer {
> > > > + compatible = "arm,armv8-timer";
> > > > + interrupts = <1 13 0xff01>,
> > > > + <1 14 0xff01>,
> > > > + <1 11 0xff01>,
> > > > + <1 10 0xff01>;
> > > > + clock-frequency = <26000000>;
> > >
> > > Please remove the clock-frequency property. Your FW should initialise
> > > CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).
> >
> > Since this comes up regularly, I think we need a dev_warn() in the arch
> > timer driver when CONFIG_ARM64.
>
> I'll ack such a patch ;)

How rude would this be?

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 2133f9d59d06..aaaf3433ccb9 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -371,7 +371,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
return;

/* Try to determine the frequency from the device tree or CNTFRQ */
- if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
+ if (IS_ENABLED(CONFIG_ARM64) ||
+ of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
if (cntbase)
arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
else

2014-11-28 14:36:34

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

On Fri, Nov 28, 2014 at 02:29:13PM +0000, Catalin Marinas wrote:
> On Thu, Nov 27, 2014 at 01:43:09PM +0000, Mark Rutland wrote:
> > On Thu, Nov 27, 2014 at 12:12:15PM +0000, Catalin Marinas wrote:
> > > On Thu, Nov 27, 2014 at 11:50:43AM +0000, Mark Rutland wrote:
> > > > On Tue, Nov 25, 2014 at 12:16:56PM +0000, Chunyan Zhang wrote:
> > > > > +
> > > > > + timer {
> > > > > + compatible = "arm,armv8-timer";
> > > > > + interrupts = <1 13 0xff01>,
> > > > > + <1 14 0xff01>,
> > > > > + <1 11 0xff01>,
> > > > > + <1 10 0xff01>;
> > > > > + clock-frequency = <26000000>;
> > > >
> > > > Please remove the clock-frequency property. Your FW should initialise
> > > > CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).
> > >
> > > Since this comes up regularly, I think we need a dev_warn() in the arch
> > > timer driver when CONFIG_ARM64.
> >
> > I'll ack such a patch ;)
>
> How rude would this be?
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 2133f9d59d06..aaaf3433ccb9 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -371,7 +371,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
> return;
>
> /* Try to determine the frequency from the device tree or CNTFRQ */
> - if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
> + if (IS_ENABLED(CONFIG_ARM64) ||
> + of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
> if (cntbase)
> arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
> else
>

Probably too rude, given it doesn't WARN() the user.

We should be extremely loud if we see the clock-frequency property on an
arm64 system. Whether or not we should ignore the property is another
matter.

Mark.

2014-11-28 14:44:23

by Will Deacon

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

On Fri, Nov 28, 2014 at 02:35:32PM +0000, Mark Rutland wrote:
> On Fri, Nov 28, 2014 at 02:29:13PM +0000, Catalin Marinas wrote:
> > On Thu, Nov 27, 2014 at 01:43:09PM +0000, Mark Rutland wrote:
> > > On Thu, Nov 27, 2014 at 12:12:15PM +0000, Catalin Marinas wrote:
> > > > On Thu, Nov 27, 2014 at 11:50:43AM +0000, Mark Rutland wrote:
> > > > > On Tue, Nov 25, 2014 at 12:16:56PM +0000, Chunyan Zhang wrote:
> > > > > > +
> > > > > > + timer {
> > > > > > + compatible = "arm,armv8-timer";
> > > > > > + interrupts = <1 13 0xff01>,
> > > > > > + <1 14 0xff01>,
> > > > > > + <1 11 0xff01>,
> > > > > > + <1 10 0xff01>;
> > > > > > + clock-frequency = <26000000>;
> > > > >
> > > > > Please remove the clock-frequency property. Your FW should initialise
> > > > > CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).
> > > >
> > > > Since this comes up regularly, I think we need a dev_warn() in the arch
> > > > timer driver when CONFIG_ARM64.
> > >
> > > I'll ack such a patch ;)
> >
> > How rude would this be?
> >
> > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> > index 2133f9d59d06..aaaf3433ccb9 100644
> > --- a/drivers/clocksource/arm_arch_timer.c
> > +++ b/drivers/clocksource/arm_arch_timer.c
> > @@ -371,7 +371,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
> > return;
> >
> > /* Try to determine the frequency from the device tree or CNTFRQ */
> > - if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
> > + if (IS_ENABLED(CONFIG_ARM64) ||
> > + of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
> > if (cntbase)
> > arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
> > else
> >
>
> Probably too rude, given it doesn't WARN() the user.

We override broken hardware ID registers all the time in device-tree without
dumping stack. Why is this any different?

> We should be extremely loud if we see the clock-frequency property on an
> arm64 system. Whether or not we should ignore the property is another
> matter.

I don't really see the point in ignoring it. We will see broken hardware
[1] and this is just preventing ourselves from working around it. I'd much
rather have arch-timers with a "clock-frequence" property than have some
other timer instead because the kernel driver is being stubborn.

Will

[1] A previous version of the Juno firmware, for example.

2014-11-28 14:47:44

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

2014-11-28 14:52:17

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

On Fri, Nov 28, 2014 at 02:44:12PM +0000, Will Deacon wrote:
> On Fri, Nov 28, 2014 at 02:35:32PM +0000, Mark Rutland wrote:

> > Probably too rude, given it doesn't WARN() the user.

> We override broken hardware ID registers all the time in device-tree without
> dumping stack. Why is this any different?

I do tend to agree that a WARN() is excessive but given the amount of
pushback on using this property on ARMv8 systems saying something would
be nice, though...

> > We should be extremely loud if we see the clock-frequency property on an
> > arm64 system. Whether or not we should ignore the property is another
> > matter.

> I don't really see the point in ignoring it. We will see broken hardware
> [1] and this is just preventing ourselves from working around it. I'd much
> rather have arch-timers with a "clock-frequence" property than have some
> other timer instead because the kernel driver is being stubborn.

...it is likely that even if there is a warning we'll end up in this
situation. If the kernel doesn't complain at all it seems totally
reasonable for people to use the feature, but just refusing to allow it
to be used at all doesn't seem like it's actually helping things.

Attachments:

(No filename) (1.17 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2014-11-28 14:59:36

by Will Deacon

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

On Fri, Nov 28, 2014 at 02:46:51PM +0000, Mark Rutland wrote:
> On Fri, Nov 28, 2014 at 02:44:12PM +0000, Will Deacon wrote:
> > On Fri, Nov 28, 2014 at 02:35:32PM +0000, Mark Rutland wrote:
> > > On Fri, Nov 28, 2014 at 02:29:13PM +0000, Catalin Marinas wrote:
> > > > On Thu, Nov 27, 2014 at 01:43:09PM +0000, Mark Rutland wrote:
> > > > > On Thu, Nov 27, 2014 at 12:12:15PM +0000, Catalin Marinas wrote:
> > > > > > On Thu, Nov 27, 2014 at 11:50:43AM +0000, Mark Rutland wrote:
> > > > > > > On Tue, Nov 25, 2014 at 12:16:56PM +0000, Chunyan Zhang wrote:
> > > > > > > > +
> > > > > > > > + timer {
> > > > > > > > + compatible = "arm,armv8-timer";
> > > > > > > > + interrupts = <1 13 0xff01>,
> > > > > > > > + <1 14 0xff01>,
> > > > > > > > + <1 11 0xff01>,
> > > > > > > > + <1 10 0xff01>;
> > > > > > > > + clock-frequency = <26000000>;
> > > > > > >
> > > > > > > Please remove the clock-frequency property. Your FW should initialise
> > > > > > > CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).
> > > > > >
> > > > > > Since this comes up regularly, I think we need a dev_warn() in the arch
> > > > > > timer driver when CONFIG_ARM64.
> > > > >
> > > > > I'll ack such a patch ;)
> > > >
> > > > How rude would this be?
> > > >
> > > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> > > > index 2133f9d59d06..aaaf3433ccb9 100644
> > > > --- a/drivers/clocksource/arm_arch_timer.c
> > > > +++ b/drivers/clocksource/arm_arch_timer.c
> > > > @@ -371,7 +371,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
> > > > return;
> > > >
> > > > /* Try to determine the frequency from the device tree or CNTFRQ */
> > > > - if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
> > > > + if (IS_ENABLED(CONFIG_ARM64) ||
> > > > + of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
> > > > if (cntbase)
> > > > arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
> > > > else
> > > >
> > >
> > > Probably too rude, given it doesn't WARN() the user.
> >
> > We override broken hardware ID registers all the time in device-tree without
> > dumping stack. Why is this any different?
>
> Exposure to guests via KVM, and the fact that it's possible to write to
> the register from EL3. This isn't so much broken HW (which cannot be
> fixed) as broken FW (which can be fixed).

I think we'll see plenty of systems where vendors are reluctant to offer
over-the-air firmware upgrades for issues that can be solved simply by
adding a property to the device-tree.

> Printing the warning gives people the chance to realise and fix the
> issue during bringup.

Print a warning, but don't dump the stack. A simple pr_warn is fine.

Will

2014-11-28 15:03:44

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

On Fri, Nov 28, 2014 at 02:44:12PM +0000, Will Deacon wrote:
> On Fri, Nov 28, 2014 at 02:35:32PM +0000, Mark Rutland wrote:
> > On Fri, Nov 28, 2014 at 02:29:13PM +0000, Catalin Marinas wrote:
> > > On Thu, Nov 27, 2014 at 01:43:09PM +0000, Mark Rutland wrote:
> > > > On Thu, Nov 27, 2014 at 12:12:15PM +0000, Catalin Marinas wrote:
> > > > > On Thu, Nov 27, 2014 at 11:50:43AM +0000, Mark Rutland wrote:
> > > > > > On Tue, Nov 25, 2014 at 12:16:56PM +0000, Chunyan Zhang wrote:
> > > > > > > +
> > > > > > > + timer {
> > > > > > > + compatible = "arm,armv8-timer";
> > > > > > > + interrupts = <1 13 0xff01>,
> > > > > > > + <1 14 0xff01>,
> > > > > > > + <1 11 0xff01>,
> > > > > > > + <1 10 0xff01>;
> > > > > > > + clock-frequency = <26000000>;
> > > > > >
> > > > > > Please remove the clock-frequency property. Your FW should initialise
> > > > > > CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).
> > > > >
> > > > > Since this comes up regularly, I think we need a dev_warn() in the arch
> > > > > timer driver when CONFIG_ARM64.
> > > >
> > > > I'll ack such a patch ;)
> > >
> > > How rude would this be?
> > >
> > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> > > index 2133f9d59d06..aaaf3433ccb9 100644
> > > --- a/drivers/clocksource/arm_arch_timer.c
> > > +++ b/drivers/clocksource/arm_arch_timer.c
> > > @@ -371,7 +371,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
> > > return;
> > >
> > > /* Try to determine the frequency from the device tree or CNTFRQ */
> > > - if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
> > > + if (IS_ENABLED(CONFIG_ARM64) ||
> > > + of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
> > > if (cntbase)
> > > arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
> > > else
> > >
> >
> > Probably too rude, given it doesn't WARN() the user.
>
> We override broken hardware ID registers all the time in device-tree without
> dumping stack. Why is this any different?

I'm not for dumping the stack, it's not relevant (just more noise).

> > We should be extremely loud if we see the clock-frequency property on an
> > arm64 system. Whether or not we should ignore the property is another
> > matter.
>
> I don't really see the point in ignoring it. We will see broken hardware
> [1] and this is just preventing ourselves from working around it. I'd much
> rather have arch-timers with a "clock-frequence" property than have some
> other timer instead because the kernel driver is being stubborn.

I agree that sooner or later we'll need a workaround (we already did for
Juno). My point is that many consider such overriding behaviour to be
the default - i.e. don't bother writing any sane value in CNTFRQ in
firmware at boot because Linux can cope without. It gets worse when
companies develop their firmware long before starting to upstream kernel
patches, so too late to fix it.

> [1] A previous version of the Juno firmware, for example.

What about CONFIG_BROKEN_FIRMWARE, default off?

In the meantime I think we can be more tolerant:

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 2133f9d59d06..87f67a93fcc7 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -376,6 +376,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
else
arch_timer_rate = arch_timer_get_cntfrq();
+ } else if (IS_ENABLED(CONFIG_ARM64)) {
+ pr_warn("Architected timer frequency overridden by DT (broken firmware?)\n");
}

/* Check the timer frequency. */

2014-11-28 15:09:23

by Will Deacon

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

On Fri, Nov 28, 2014 at 03:03:26PM +0000, Catalin Marinas wrote:
> On Fri, Nov 28, 2014 at 02:44:12PM +0000, Will Deacon wrote:
> > On Fri, Nov 28, 2014 at 02:35:32PM +0000, Mark Rutland wrote:
> > > On Fri, Nov 28, 2014 at 02:29:13PM +0000, Catalin Marinas wrote:
> > > > On Thu, Nov 27, 2014 at 01:43:09PM +0000, Mark Rutland wrote:
> > > > > On Thu, Nov 27, 2014 at 12:12:15PM +0000, Catalin Marinas wrote:
> > > > > > On Thu, Nov 27, 2014 at 11:50:43AM +0000, Mark Rutland wrote:
> > > > > > > On Tue, Nov 25, 2014 at 12:16:56PM +0000, Chunyan Zhang wrote:
> > > > > > > > +
> > > > > > > > + timer {
> > > > > > > > + compatible = "arm,armv8-timer";
> > > > > > > > + interrupts = <1 13 0xff01>,
> > > > > > > > + <1 14 0xff01>,
> > > > > > > > + <1 11 0xff01>,
> > > > > > > > + <1 10 0xff01>;
> > > > > > > > + clock-frequency = <26000000>;
> > > > > > >
> > > > > > > Please remove the clock-frequency property. Your FW should initialise
> > > > > > > CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).
> > > > > >
> > > > > > Since this comes up regularly, I think we need a dev_warn() in the arch
> > > > > > timer driver when CONFIG_ARM64.
> > > > >
> > > > > I'll ack such a patch ;)
> > > >
> > > > How rude would this be?
> > > >
> > > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> > > > index 2133f9d59d06..aaaf3433ccb9 100644
> > > > --- a/drivers/clocksource/arm_arch_timer.c
> > > > +++ b/drivers/clocksource/arm_arch_timer.c
> > > > @@ -371,7 +371,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
> > > > return;
> > > >
> > > > /* Try to determine the frequency from the device tree or CNTFRQ */
> > > > - if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
> > > > + if (IS_ENABLED(CONFIG_ARM64) ||
> > > > + of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
> > > > if (cntbase)
> > > > arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
> > > > else
> > > >
> > >
> > > Probably too rude, given it doesn't WARN() the user.
> >
> > We override broken hardware ID registers all the time in device-tree without
> > dumping stack. Why is this any different?
>
> I'm not for dumping the stack, it's not relevant (just more noise).
>
> > > We should be extremely loud if we see the clock-frequency property on an
> > > arm64 system. Whether or not we should ignore the property is another
> > > matter.
> >
> > I don't really see the point in ignoring it. We will see broken hardware
> > [1] and this is just preventing ourselves from working around it. I'd much
> > rather have arch-timers with a "clock-frequence" property than have some
> > other timer instead because the kernel driver is being stubborn.
>
> I agree that sooner or later we'll need a workaround (we already did for
> Juno). My point is that many consider such overriding behaviour to be
> the default - i.e. don't bother writing any sane value in CNTFRQ in
> firmware at boot because Linux can cope without. It gets worse when
> companies develop their firmware long before starting to upstream kernel
> patches, so too late to fix it.
>
> > [1] A previous version of the Juno firmware, for example.
>
> What about CONFIG_BROKEN_FIRMWARE, default off?

I'd rather have a `firmware test' module, which could be as noisy as it
likes when it finds issues like this. It could also do things like fuzz the
PSCI interface.

> In the meantime I think we can be more tolerant:
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 2133f9d59d06..87f67a93fcc7 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -376,6 +376,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
> arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
> else
> arch_timer_rate = arch_timer_get_cntfrq();
> + } else if (IS_ENABLED(CONFIG_ARM64)) {
> + pr_warn("Architected timer frequency overridden by DT (broken firmware?)\n");
> }

That looks sensible. It would be interesting to print the value of CNTFRQ
too.

Will

2014-11-28 16:41:52

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

[...]

> In the meantime I think we can be more tolerant:
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 2133f9d59d06..87f67a93fcc7 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -376,6 +376,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
> arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
> else
> arch_timer_rate = arch_timer_get_cntfrq();
> + } else if (IS_ENABLED(CONFIG_ARM64)) {
> + pr_warn("Architected timer frequency overridden by DT (broken firmware?)\n");
> }

That looks good to me. On a related note, it looks like my documentation
patch [1] from a while back fell by the wayside. Does anyone fancy
picking that up to go with this?

Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/282830.html

2014-11-28 17:24:48

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

On Fri, Nov 28, 2014 at 04:40:47PM +0000, Mark Rutland wrote:
> [...]
>
> > In the meantime I think we can be more tolerant:
> >
> > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> > index 2133f9d59d06..87f67a93fcc7 100644
> > --- a/drivers/clocksource/arm_arch_timer.c
> > +++ b/drivers/clocksource/arm_arch_timer.c
> > @@ -376,6 +376,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
> > arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
> > else
> > arch_timer_rate = arch_timer_get_cntfrq();
> > + } else if (IS_ENABLED(CONFIG_ARM64)) {
> > + pr_warn("Architected timer frequency overridden by DT (broken firmware?)\n");
> > }
>
> That looks good to me. On a related note, it looks like my documentation
> patch [1] from a while back fell by the wayside. Does anyone fancy
> picking that up to go with this?

I can put the two together (keeping you as author). Do we still need the
IS_ENABLED(CONFIG_ARM64) part here or we extend the warning to to arm32
as well?

--
Catalin

2014-12-03 09:16:58

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

>
>> + gic: interrupt-controller@12001000 {
>> + compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";
>> + #interrupt-cells = <3>;
>> + interrupt-controller;
>> + reg = <0 0x12001000 0 0x1000>,
>> + <0 0x12002000 0 0x1000>,
>> + <0 0x12004000 0 0x2000>,
>> + <0 0x12006000 0 0x2000>;
>> + };
>
> I believe GICC should be 8KiB here.
>
Yes, I'll correct it in next patch.
>> +
>> + psci {
>> + compatible = "arm,psci-0.2";
>> + method = "smc";
>> + };
>
> Do you have a complete PSCI 0.2 implementation (e.g. are all the
> mandatory functions implemented)?
>
I was using the latest boot-wrapper as the bootloader for test, I'll
add a compatible string "arm,psci" in v4.
We'll use u-boot on real hardware, the support for PSCI in our u-boot
is in progress.
We're intending to implement psci-0.1 at first, and psci-0.2 for the next step.

> I take it CPUs enter the kernel at EL2?
>
yes, we've just do like this.

> How have you tested this?

I'll test it on the SC9836 FPGA board before next submitting.

Thanks for your patient review.
Chunyan

2014-12-03 09:19:48

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

2014-11-27 2:29 GMT+08:00 Murali Karicheri <[email protected]>:
>
> On 11/25/2014 07:16 AM, 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.
>>
>> [...]

>>
>> +++ b/drivers/tty/serial/sprd_serial.c
>> @@ -0,0 +1,752 @@
>> +/*
>> + * Copyright (C) 2012 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.
>> + */
>> +
>> +#include<linux/module.h>
>> +#include<linux/tty.h>
>> +#include<linux/ioport.h>
>> +#include<linux/console.h>
>> +#include<linux/platform_device.h>
>> +#include<linux/tty_flip.h>
>> +#include<linux/serial_core.h>
>> +#include<linux/serial.h>
>> +#include<linux/delay.h>
>> +#include<linux/io.h>
>> +#include<asm/irq.h>
>> +#include<linux/slab.h>
>> +#include<linux/of.h>
>> +#include<linux/kernel.h>
>> +#include<linux/slab.h>
>> +#include<linux/clk.h>
>
> How about sorting this includes? asm/irq.h go first followed linux/ in alphabatical order?

There are a few compile warnings if I moved asm/irq.h to the top of
all included files.
Warning details are below:

In file included from drivers/tty/serial/sprd_serial.c:14:0:
./arch/arm64/include/asm/irq.h:6:39: warning: ‘struct pt_regs’
declared inside parameter list [enabled by default]
extern void (*handle_arch_irq)(struct pt_regs *);
^
./arch/arm64/include/asm/irq.h:6:39: warning: its scope is only this
definition or declaration, which is probably not what you want
[enabled by default]
./arch/arm64/include/asm/irq.h:8:54: warning: ‘struct pt_regs’
declared inside parameter list [enabled by default]
extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));

Thanks,
Chunyan

2014-12-03 09:51:07

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

On Wednesday 03 December 2014 17:17:13 Lyra Zhang wrote:

> 2014-11-27 2:29 GMT+08:00 Murali Karicheri <[email protected]>:
> > How about sorting this includes? asm/irq.h go first followed linux/ in alphabatical order?
>
> There are a few compile warnings if I moved asm/irq.h to the top of
> all included files.

The order that Murali meant is

- first all linux/*.h headers, sorted alphabetically
- then all asm/*.h headers, again sorted alphabetically

This is the recommended style in general.

> Warning details are below:
>
> In file included from drivers/tty/serial/sprd_serial.c:14:0:
> ./arch/arm64/include/asm/irq.h:6:39: warning: ‘struct pt_regs’
> declared inside parameter list [enabled by default]
> extern void (*handle_arch_irq)(struct pt_regs *);
> ^
> ./arch/arm64/include/asm/irq.h:6:39: warning: its scope is only this
> definition or declaration, which is probably not what you want
> [enabled by default]
> ./arch/arm64/include/asm/irq.h:8:54: warning: ‘struct pt_regs’
> declared inside parameter list [enabled by default]
> extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
>

I would consider this a (minor) bug in asm/irq.h. If you don't mind,
please submit a patch to add a line with 'struct pt_regs;' to
asm/irq.h.

Arnd

2014-12-03 10:11:57

by Russell King - ARM Linux

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

On Wed, Dec 03, 2014 at 10:50:17AM +0100, Arnd Bergmann wrote:
> On Wednesday 03 December 2014 17:17:13 Lyra Zhang wrote:
>
> > 2014-11-27 2:29 GMT+08:00 Murali Karicheri <[email protected]>:
> > > How about sorting this includes? asm/irq.h go first followed linux/ in alphabatical order?
> >
> > There are a few compile warnings if I moved asm/irq.h to the top of
> > all included files.
>
> The order that Murali meant is
>
> - first all linux/*.h headers, sorted alphabetically
> - then all asm/*.h headers, again sorted alphabetically
>
> This is the recommended style in general.
>
> > Warning details are below:
> >
> > In file included from drivers/tty/serial/sprd_serial.c:14:0:
> > ./arch/arm64/include/asm/irq.h:6:39: warning: ‘struct pt_regs’
> > declared inside parameter list [enabled by default]
> > extern void (*handle_arch_irq)(struct pt_regs *);
> > ^
> > ./arch/arm64/include/asm/irq.h:6:39: warning: its scope is only this
> > definition or declaration, which is probably not what you want
> > [enabled by default]
> > ./arch/arm64/include/asm/irq.h:8:54: warning: ‘struct pt_regs’
> > declared inside parameter list [enabled by default]
> > extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
> >
>
> I would consider this a (minor) bug in asm/irq.h. If you don't mind,
> please submit a patch to add a line with 'struct pt_regs;' to
> asm/irq.h.

A better question is: why is a modern driver using asm/irq.h in the
first place.

We used to include that file when platforms defined IRQ numbers
statically, but modern platforms don't do that, so it shouldn't be
required anymore.

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

2014-12-03 12:08:16

[permalink] [raw]

Subject: Re: [PATCH v3 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

2014-12-03 18:11 GMT+08:00 Russell King - ARM Linux <[email protected]>:
> On Wed, Dec 03, 2014 at 10:50:17AM +0100, Arnd Bergmann wrote:
>> On Wednesday 03 December 2014 17:17:13 Lyra Zhang wrote:
>>
>> > 2014-11-27 2:29 GMT+08:00 Murali Karicheri <[email protected]>:
>> > > How about sorting this includes? asm/irq.h go first followed linux/ in alphabatical order?
>> >
>> > There are a few compile warnings if I moved asm/irq.h to the top of
>> > all included files.
>>
>> The order that Murali meant is
>>
>> - first all linux/*.h headers, sorted alphabetically
>> - then all asm/*.h headers, again sorted alphabetically
>>
>> This is the recommended style in general.
>>
>> > Warning details are below:
>> >
>> > In file included from drivers/tty/serial/sprd_serial.c:14:0:
>> > ./arch/arm64/include/asm/irq.h:6:39: warning: ‘struct pt_regs’
>> > declared inside parameter list [enabled by default]
>> > extern void (*handle_arch_irq)(struct pt_regs *);
>> > ^
>> > ./arch/arm64/include/asm/irq.h:6:39: warning: its scope is only this
>> > definition or declaration, which is probably not what you want
>> > [enabled by default]
>> > ./arch/arm64/include/asm/irq.h:8:54: warning: ‘struct pt_regs’
>> > declared inside parameter list [enabled by default]
>> > extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
>> >
>>
>> I would consider this a (minor) bug in asm/irq.h. If you don't mind,
>> please submit a patch to add a line with 'struct pt_regs;' to
>> asm/irq.h.
>
> A better question is: why is a modern driver using asm/irq.h in the
> first place.
>
> We used to include that file when platforms defined IRQ numbers
> statically, but modern platforms don't do that, so it shouldn't be
> required anymore.
>

OK, I see. I'll remove it in v4.

I'm very sorry that I didn't check it carefully enough and troubled
many people for this problem.
This file included the asm/irq.h before I revised it for upstreaming,
I also could not find this file was no longer needed to include.
Thank you for your reminder, I'll avoid making the same mistake, and
I'll also share my experience with my colleagues to keep them from
having this mistake in the future.

Best regards,
Chunyan

2014-12-03 12:15:54