diff mbox

[U-Boot,2/4] dm: serial: Tidy up the pl01x driver

Message ID 1411428659-6823-3-git-send-email-sjg@chromium.org
State Awaiting Upstream
Delegated to: Simon Glass
Headers show

Commit Message

Simon Glass Sept. 22, 2014, 11:30 p.m. UTC 
Adjust the driver so that leaf functions take a pointer to the serial port
register base. Put all the global configuration in the init function, and
use the same settings from then on.

This makes it much easier to move to driver model without duplicating the
code, since with driver model we use platform data rather than global
settings.

The driver is compiled with either the CONFIG_PL010_SERIAL or
CONFIG_PL011_SERIAL option and this determines the uart type. With driver
model this needs to come in from platform data, so create a new
CONFIG_PL01X_SERIAL config which brings in the driver, and adjust the
driver to support both peripheral variants.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/serial/Makefile                            |   5 +-
 drivers/serial/serial_pl01x.c                      | 300 +++++++++++----------
 .../{serial_pl01x.h => serial_pl01x_internal.h}    |   0
 include/serial_pl01x.h                             |  27 ++
 4 files changed, 192 insertions(+), 140 deletions(-)
 rename drivers/serial/{serial_pl01x.h => serial_pl01x_internal.h} (100%)
 create mode 100644 include/serial_pl01x.h

Comments

Simon Glass Oct. 23, 2014, 3:07 a.m. UTC | #1 
On 22 September 2014 17:30, Simon Glass <sjg@chromium.org> wrote:
> Adjust the driver so that leaf functions take a pointer to the serial port
> register base. Put all the global configuration in the init function, and
> use the same settings from then on.
>
> This makes it much easier to move to driver model without duplicating the
> code, since with driver model we use platform data rather than global
> settings.
>
> The driver is compiled with either the CONFIG_PL010_SERIAL or
> CONFIG_PL011_SERIAL option and this determines the uart type. With driver
> model this needs to come in from platform data, so create a new
> CONFIG_PL01X_SERIAL config which brings in the driver, and adjust the
> driver to support both peripheral variants.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  drivers/serial/Makefile                            |   5 +-
>  drivers/serial/serial_pl01x.c                      | 300 +++++++++++----------
>  .../{serial_pl01x.h => serial_pl01x_internal.h}    |   0
>  include/serial_pl01x.h                             |  27 ++
>  4 files changed, 192 insertions(+), 140 deletions(-)
>  rename drivers/serial/{serial_pl01x.h => serial_pl01x_internal.h} (100%)
>  create mode 100644 include/serial_pl01x.h
>

Applied to u-boot-dm/master.
Linus Walleij April 21, 2015, 11:56 a.m. UTC | #2 
On Tue, Sep 23, 2014 at 1:30 AM, Simon Glass <sjg@chromium.org> wrote:

> Adjust the driver so that leaf functions take a pointer to the serial port
> register base. Put all the global configuration in the init function, and
> use the same settings from then on.
>
> This makes it much easier to move to driver model without duplicating the
> code, since with driver model we use platform data rather than global
> settings.
>
> The driver is compiled with either the CONFIG_PL010_SERIAL or
> CONFIG_PL011_SERIAL option and this determines the uart type. With driver
> model this needs to come in from platform data, so create a new
> CONFIG_PL01X_SERIAL config which brings in the driver, and adjust the
> driver to support both peripheral variants.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Unfortunately this patch regresses PL010 (Integrator/AP), and I
suspect it has never been tested on real-world PL010 hardware?

I'm hunting around to figure out what is causing it, any hints welcome.

Yours,
Linus Walleij
Linus Walleij April 21, 2015, noon UTC | #3 
On Tue, Apr 21, 2015 at 1:56 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Sep 23, 2014 at 1:30 AM, Simon Glass <sjg@chromium.org> wrote:
>
>> Adjust the driver so that leaf functions take a pointer to the serial port
>> register base. Put all the global configuration in the init function, and
>> use the same settings from then on.
>>
>> This makes it much easier to move to driver model without duplicating the
>> code, since with driver model we use platform data rather than global
>> settings.
>>
>> The driver is compiled with either the CONFIG_PL010_SERIAL or
>> CONFIG_PL011_SERIAL option and this determines the uart type. With driver
>> model this needs to come in from platform data, so create a new
>> CONFIG_PL01X_SERIAL config which brings in the driver, and adjust the
>> driver to support both peripheral variants.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Unfortunately this patch regresses PL010 (Integrator/AP), and I
> suspect it has never been tested on real-world PL010 hardware?
>
> I'm hunting around to figure out what is causing it, any hints welcome.

FYI: U-Boot does come up, but trying to switch baudrate from
the default 38400 to 115200 with set baudrate 115200 hangs,
it's as if the change never hits the hardware so the routine looking
for a CR after the baudrate switch just waits. If I switch the terminal
back to 38400 and hit enter it goes through and works again, and
U-Boot thinks it has successfully switched to 115200, while it hasn't,
really.

Kind of frustrating since I upload kernels over the serial port :P

Yours,
Linus Walleij
diff mbox

Patch

diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
index 9ac3496..edf6936 100644
--- a/drivers/serial/Makefile
+++ b/drivers/serial/Makefile
@@ -7,8 +7,11 @@ 
 
 ifdef CONFIG_DM_SERIAL
 obj-y += serial-uclass.o
+obj-$(CONFIG_PL01X_SERIAL) += serial_pl01x.o
 else
 obj-y += serial.o
+obj-$(CONFIG_PL010_SERIAL) += serial_pl01x.o
+obj-$(CONFIG_PL011_SERIAL) += serial_pl01x.o
 obj-$(CONFIG_SYS_NS16550_SERIAL) += serial_ns16550.o
 endif
 
@@ -25,8 +28,6 @@  obj-$(CONFIG_IMX_SERIAL) += serial_imx.o
 obj-$(CONFIG_KS8695_SERIAL) += serial_ks8695.o
 obj-$(CONFIG_MAX3100_SERIAL) += serial_max3100.o
 obj-$(CONFIG_MXC_UART) += serial_mxc.o
-obj-$(CONFIG_PL010_SERIAL) += serial_pl01x.o
-obj-$(CONFIG_PL011_SERIAL) += serial_pl01x.o
 obj-$(CONFIG_PXA_SERIAL) += serial_pxa.o
 obj-$(CONFIG_SA1100_SERIAL) += serial_sa1100.o
 obj-$(CONFIG_S3C24X0_SERIAL) += serial_s3c24x0.o
diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c
index dfb610e..665a0e4 100644
--- a/drivers/serial/serial_pl01x.c
+++ b/drivers/serial/serial_pl01x.c
@@ -12,125 +12,86 @@ 
 /* Simple U-Boot driver for the PrimeCell PL010/PL011 UARTs */
 
 #include <common.h>
+#include <errno.h>
 #include <watchdog.h>
 #include <asm/io.h>
 #include <serial.h>
+#include <serial_pl01x.h>
 #include <linux/compiler.h>
-#include "serial_pl01x.h"
+#include "serial_pl01x_internal.h"
 
-/*
- * Integrator AP has two UARTs, we use the first one, at 38400-8-N-1
- * Integrator CP has two UARTs, use the first one, at 38400-8-N-1
- * Versatile PB has four UARTs.
- */
-#define CONSOLE_PORT CONFIG_CONS_INDEX
 static volatile unsigned char *const port[] = CONFIG_PL01x_PORTS;
+static enum pl01x_type pl01x_type __attribute__ ((section(".data")));
+static struct pl01x_regs *base_regs __attribute__ ((section(".data")));
 #define NUM_PORTS (sizeof(port)/sizeof(port[0]))
 
-static void pl01x_putc (int portnum, char c);
-static int pl01x_getc (int portnum);
-static int pl01x_tstc (int portnum);
-unsigned int baudrate = CONFIG_BAUDRATE;
 DECLARE_GLOBAL_DATA_PTR;
 
-static struct pl01x_regs *pl01x_get_regs(int portnum)
+static int pl01x_putc(struct pl01x_regs *regs, char c)
 {
-	return (struct pl01x_regs *) port[portnum];
-}
-
-#ifdef CONFIG_PL010_SERIAL
-
-static int pl01x_serial_init(void)
-{
-	struct pl01x_regs *regs = pl01x_get_regs(CONSOLE_PORT);
-	unsigned int divisor;
-
-	/* First, disable everything */
-	writel(0, &regs->pl010_cr);
+	/* Wait until there is space in the FIFO */
+	if (readl(&regs->fr) & UART_PL01x_FR_TXFF)
+		return -EAGAIN;
 
-	/* Set baud rate */
-	switch (baudrate) {
-	case 9600:
-		divisor = UART_PL010_BAUD_9600;
-		break;
+	/* Send the character */
+	writel(c, &regs->dr);
 
-	case 19200:
-		divisor = UART_PL010_BAUD_9600;
-		break;
+	return 0;
+}
 
-	case 38400:
-		divisor = UART_PL010_BAUD_38400;
-		break;
+static int pl01x_getc(struct pl01x_regs *regs)
+{
+	unsigned int data;
 
-	case 57600:
-		divisor = UART_PL010_BAUD_57600;
-		break;
+	/* Wait until there is data in the FIFO */
+	if (readl(&regs->fr) & UART_PL01x_FR_RXFE)
+		return -EAGAIN;
 
-	case 115200:
-		divisor = UART_PL010_BAUD_115200;
-		break;
+	data = readl(&regs->dr);
 
-	default:
-		divisor = UART_PL010_BAUD_38400;
+	/* Check for an error flag */
+	if (data & 0xFFFFFF00) {
+		/* Clear the error */
+		writel(0xFFFFFFFF, &regs->ecr);
+		return -1;
 	}
 
-	writel((divisor & 0xf00) >> 8, &regs->pl010_lcrm);
-	writel(divisor & 0xff, &regs->pl010_lcrl);
-
-	/* Set the UART to be 8 bits, 1 stop bit, no parity, fifo enabled */
-	writel(UART_PL010_LCRH_WLEN_8 | UART_PL010_LCRH_FEN, &regs->pl010_lcrh);
-
-	/* Finally, enable the UART */
-	writel(UART_PL010_CR_UARTEN, &regs->pl010_cr);
-
-	return 0;
+	return (int) data;
 }
 
-#endif /* CONFIG_PL010_SERIAL */
-
-#ifdef CONFIG_PL011_SERIAL
+static int pl01x_tstc(struct pl01x_regs *regs)
+{
+	WATCHDOG_RESET();
+	return !(readl(&regs->fr) & UART_PL01x_FR_RXFE);
+}
 
-static int pl01x_serial_init(void)
+static int pl01x_generic_serial_init(struct pl01x_regs *regs,
+				     enum pl01x_type type)
 {
-	struct pl01x_regs *regs = pl01x_get_regs(CONSOLE_PORT);
-	unsigned int temp;
-	unsigned int divider;
-	unsigned int remainder;
-	unsigned int fraction;
 	unsigned int lcr;
 
 #ifdef CONFIG_PL011_SERIAL_FLUSH_ON_INIT
-	/* Empty RX fifo if necessary */
-	if (readl(&regs->pl011_cr) & UART_PL011_CR_UARTEN) {
-		while (!(readl(&regs->fr) & UART_PL01x_FR_RXFE))
-			readl(&regs->dr);
+	if (type == TYPE_PL011) {
+		/* Empty RX fifo if necessary */
+		if (readl(&regs->pl011_cr) & UART_PL011_CR_UARTEN) {
+			while (!(readl(&regs->fr) & UART_PL01x_FR_RXFE))
+				readl(&regs->dr);
+		}
 	}
 #endif
 
 	/* First, disable everything */
-	writel(0, &regs->pl011_cr);
-
-	/*
-	 * Set baud rate
-	 *
-	 * IBRD = UART_CLK / (16 * BAUD_RATE)
-	 * FBRD = RND((64 * MOD(UART_CLK,(16 * BAUD_RATE))) / (16 * BAUD_RATE))
-	 */
-	temp = 16 * baudrate;
-	divider = CONFIG_PL011_CLOCK / temp;
-	remainder = CONFIG_PL011_CLOCK % temp;
-	temp = (8 * remainder) / baudrate;
-	fraction = (temp >> 1) + (temp & 1);
-
-	writel(divider, &regs->pl011_ibrd);
-	writel(fraction, &regs->pl011_fbrd);
+	writel(0, &regs->pl010_cr);
 
 	/* Set the UART to be 8 bits, 1 stop bit, no parity, fifo enabled */
 	lcr = UART_PL011_LCRH_WLEN_8 | UART_PL011_LCRH_FEN;
 	writel(lcr, &regs->pl011_lcrh);
 
+	switch (type) {
+	case TYPE_PL010:
+		break;
+	case TYPE_PL011: {
 #ifdef CONFIG_PL011_SERIAL_RLCR
-	{
 		int i;
 
 		/*
@@ -144,90 +105,151 @@  static int pl01x_serial_init(void)
 		writel(lcr, &regs->pl011_rlcr);
 		/* lcrh needs to be set again for change to be effective */
 		writel(lcr, &regs->pl011_lcrh);
-	}
 #endif
-	/* Finally, enable the UART */
-	writel(UART_PL011_CR_UARTEN | UART_PL011_CR_TXE | UART_PL011_CR_RXE |
-	       UART_PL011_CR_RTS, &regs->pl011_cr);
+		break;
+	}
+	default:
+		return -EINVAL;
+	}
 
 	return 0;
 }
 
-#endif /* CONFIG_PL011_SERIAL */
-
-static void pl01x_serial_putc(const char c)
+static int pl01x_generic_setbrg(struct pl01x_regs *regs, enum pl01x_type type,
+				int clock, int baudrate)
 {
-	if (c == '\n')
-		pl01x_putc (CONSOLE_PORT, '\r');
+	switch (type) {
+	case TYPE_PL010: {
+		unsigned int divisor;
+
+		switch (baudrate) {
+		case 9600:
+			divisor = UART_PL010_BAUD_9600;
+			break;
+		case 19200:
+			divisor = UART_PL010_BAUD_9600;
+			break;
+		case 38400:
+			divisor = UART_PL010_BAUD_38400;
+			break;
+		case 57600:
+			divisor = UART_PL010_BAUD_57600;
+			break;
+		case 115200:
+			divisor = UART_PL010_BAUD_115200;
+			break;
+		default:
+			divisor = UART_PL010_BAUD_38400;
+		}
+
+		writel((divisor & 0xf00) >> 8, &regs->pl010_lcrm);
+		writel(divisor & 0xff, &regs->pl010_lcrl);
+
+		/* Finally, enable the UART */
+		writel(UART_PL010_CR_UARTEN, &regs->pl010_cr);
+		break;
+	}
+	case TYPE_PL011: {
+		unsigned int temp;
+		unsigned int divider;
+		unsigned int remainder;
+		unsigned int fraction;
 
-	pl01x_putc (CONSOLE_PORT, c);
-}
+		/*
+		* Set baud rate
+		*
+		* IBRD = UART_CLK / (16 * BAUD_RATE)
+		* FBRD = RND((64 * MOD(UART_CLK,(16 * BAUD_RATE)))
+		*		/ (16 * BAUD_RATE))
+		*/
+		temp = 16 * baudrate;
+		divider = clock / temp;
+		remainder = clock % temp;
+		temp = (8 * remainder) / baudrate;
+		fraction = (temp >> 1) + (temp & 1);
+
+		writel(divider, &regs->pl011_ibrd);
+		writel(fraction, &regs->pl011_fbrd);
+
+		/* Finally, enable the UART */
+		writel(UART_PL011_CR_UARTEN | UART_PL011_CR_TXE |
+		       UART_PL011_CR_RXE | UART_PL011_CR_RTS, &regs->pl011_cr);
+		break;
+	}
+	default:
+		return -EINVAL;
+	}
 
-static int pl01x_serial_getc(void)
-{
-	return pl01x_getc (CONSOLE_PORT);
+	return 0;
 }
 
-static int pl01x_serial_tstc(void)
+#ifndef CONFIG_DM_SERIAL
+static void pl01x_serial_init_baud(int baudrate)
 {
-	return pl01x_tstc (CONSOLE_PORT);
+	int clock = 0;
+
+#if defined(CONFIG_PL010_SERIAL)
+	pl01x_type = TYPE_PL010;
+#elif defined(CONFIG_PL011_SERIAL)
+	pl01x_type = TYPE_PL011;
+	clock = CONFIG_PL011_CLOCK;
+#endif
+	base_regs = (struct pl01x_regs *)port[CONFIG_CONS_INDEX];
+
+	pl01x_generic_serial_init(base_regs, pl01x_type);
+	pl01x_generic_setbrg(base_regs, TYPE_PL010, clock, baudrate);
 }
 
-static void pl01x_serial_setbrg(void)
+/*
+ * Integrator AP has two UARTs, we use the first one, at 38400-8-N-1
+ * Integrator CP has two UARTs, use the first one, at 38400-8-N-1
+ * Versatile PB has four UARTs.
+ */
+int pl01x_serial_init(void)
 {
-	struct pl01x_regs *regs = pl01x_get_regs(CONSOLE_PORT);
+	pl01x_serial_init_baud(CONFIG_BAUDRATE);
 
-	baudrate = gd->baudrate;
-	/*
-	 * Flush FIFO and wait for non-busy before changing baudrate to avoid
-	 * crap in console
-	 */
-	while (!(readl(&regs->fr) & UART_PL01x_FR_TXFE))
-		WATCHDOG_RESET();
-	while (readl(&regs->fr) & UART_PL01x_FR_BUSY)
-		WATCHDOG_RESET();
-	serial_init();
+	return 0;
 }
 
-static void pl01x_putc (int portnum, char c)
+static void pl01x_serial_putc(const char c)
 {
-	struct pl01x_regs *regs = pl01x_get_regs(portnum);
-
-	/* Wait until there is space in the FIFO */
-	while (readl(&regs->fr) & UART_PL01x_FR_TXFF)
-		WATCHDOG_RESET();
+	if (c == '\n')
+		while (pl01x_putc(base_regs, '\r') == -EAGAIN);
 
-	/* Send the character */
-	writel(c, &regs->dr);
+	while (pl01x_putc(base_regs, c) == -EAGAIN);
 }
 
-static int pl01x_getc (int portnum)
+static int pl01x_serial_getc(void)
 {
-	struct pl01x_regs *regs = pl01x_get_regs(portnum);
-	unsigned int data;
+	while (1) {
+		int ch = pl01x_getc(base_regs);
 
-	/* Wait until there is data in the FIFO */
-	while (readl(&regs->fr) & UART_PL01x_FR_RXFE)
-		WATCHDOG_RESET();
+		if (ch == -EAGAIN) {
+			WATCHDOG_RESET();
+			continue;
+		}
 
-	data = readl(&regs->dr);
-
-	/* Check for an error flag */
-	if (data & 0xFFFFFF00) {
-		/* Clear the error */
-		writel(0xFFFFFFFF, &regs->ecr);
-		return -1;
+		return ch;
 	}
-
-	return (int) data;
 }
 
-static int pl01x_tstc (int portnum)
+static int pl01x_serial_tstc(void)
 {
-	struct pl01x_regs *regs = pl01x_get_regs(portnum);
+	return pl01x_tstc(base_regs);
+}
 
-	WATCHDOG_RESET();
-	return !(readl(&regs->fr) & UART_PL01x_FR_RXFE);
+static void pl01x_serial_setbrg(void)
+{
+	/*
+	 * Flush FIFO and wait for non-busy before changing baudrate to avoid
+	 * crap in console
+	 */
+	while (!(readl(&base_regs->fr) & UART_PL01x_FR_TXFE))
+		WATCHDOG_RESET();
+	while (readl(&base_regs->fr) & UART_PL01x_FR_BUSY)
+		WATCHDOG_RESET();
+	pl01x_serial_init_baud(gd->baudrate);
 }
 
 static struct serial_device pl01x_serial_drv = {
@@ -250,3 +272,5 @@  __weak struct serial_device *default_serial_console(void)
 {
 	return &pl01x_serial_drv;
 }
+
+#endif /* nCONFIG_DM_SERIAL */
diff --git a/drivers/serial/serial_pl01x.h b/drivers/serial/serial_pl01x_internal.h
similarity index 100%
rename from drivers/serial/serial_pl01x.h
rename to drivers/serial/serial_pl01x_internal.h
diff --git a/include/serial_pl01x.h b/include/serial_pl01x.h
new file mode 100644
index 0000000..5e068f3
--- /dev/null
+++ b/include/serial_pl01x.h
@@ -0,0 +1,27 @@ 
+/*
+ * Copyright (c) 2014 Google, Inc
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef __serial_pl01x_h
+#define __serial_pl01x_h
+
+enum pl01x_type {
+	TYPE_PL010,
+	TYPE_PL011,
+};
+
+/*
+ *Information about a serial port
+ *
+ * @base: Register base address
+ * @type: Port type
+ * @clock: Input clock rate, used for calculating the baud rate divisor
+ */
+struct pl01x_serial_platdata {
+	unsigned long base;
+	enum pl01x_type type;
+	unsigned int clock;
+};
+
+#endif