[U-Boot] [PATCH] ns16550: Clean up & unify register accessors

Paul Burton paul.burton at imgtec.com
Tue Feb 2 15:12:24 CET 2016


This patch starts to clean up the ns16550 driver by modifying the way in
which registers are accessed such that much of the code can be shared by
both systems using device model & systems that don't. The basic approach
is that we represent the type of register access to use with IORESOURCE
flags from linux/ioport.h. On non-DM systems the existing config macros
are used to select an appropriate value. Since this is constant at
compile time the code generated by the compiler shouldn't suffer. For
systems using DM the access type is taken from dev_get_addr_flags &
stored in the platform data struct. Whilst this will affect the
generated code, it allows for the type of access to be dynamic &
selected based upon the content of the device tree.

The debug UART support is left as-is, since fitting that in with this
rework seemed impractical.

TODO: Handle endianness for 32b memory accesses

Cc: Simon Glass <sjg at chromium.org>
Signed-off-by: Paul Burton <paul.burton at imgtec.com>
---
Hi Simon, any thoughts on this? It's not "done" yet but any feedback would be
appreciated, even if it's "stop digging!".
---
 drivers/serial/ns16550.c | 313 ++++++++++++++++++++++++++++++-----------------
 include/ns16550.h        |   9 ++
 2 files changed, 212 insertions(+), 110 deletions(-)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 93dad33..3a15a57 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -11,11 +11,142 @@
 #include <ns16550.h>
 #include <serial.h>
 #include <watchdog.h>
+#include <linux/ioport.h>
+#include <linux/log2.h>
 #include <linux/types.h>
 #include <asm/io.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
+static unsigned get_access_type(struct NS16550 *port)
+{
+	if (config_enabled(CONFIG_DM_SERIAL)) {
+		struct ns16550_platdata *plat = ns16550_plat(port);
+
+		return plat->access_flags;
+	}
+
+	/*
+	 * For systems which aren't yet using device model, determine
+	 * the access type based upon config macros.
+	 */
+
+	if (config_enabled(CONFIG_SYS_NS16550_PORT_MAPPED))
+		return IORESOURCE_IO;
+
+	if (config_enabled(CONFIG_SYS_NS16550_MEM32))
+		return IORESOURCE_MEM | IORESOURCE_MEM_32BIT;
+
+	return IORESOURCE_MEM | IORESOURCE_MEM_8BIT;
+}
+
+static void *get_reg_addr(struct NS16550 *port, unsigned idx, unsigned access_type)
+{
+	void *base, *reg;
+	unsigned shift;
+
+	if (config_enabled(CONFIG_DM_SERIAL)) {
+		struct ns16550_platdata *plat = ns16550_plat(port);
+
+		if (access_type == IORESOURCE_IO)
+			base = (void *)plat->base;
+		else
+			base = map_physmem(plat->base, 0, MAP_NOCACHE);
+
+		shift = plat->reg_shift;
+	} else {
+		base = port;
+		shift = ilog2(abs(CONFIG_SYS_NS16550_REG_SIZE));
+	}
+
+	reg = base + (idx << shift);
+
+	if ((access_type == IORESOURCE_MEM_8BIT) &&
+		config_enabled(CONFIG_SYS_BIG_ENDIAN)) {
+		/* offset reg to account for endianness */
+		reg += (1 << shift) - 1;
+	}
+
+	return reg;
+}
+
+static inline u8 read_reg(struct NS16550 *port, unsigned idx)
+{
+	unsigned access_type;
+	void *reg;
+
+	access_type = get_access_type(port);
+	reg = get_reg_addr(port, idx, access_type);
+
+	if ((access_type & IORESOURCE_TYPE_BITS) == IORESOURCE_IO)
+		return inb((ulong)reg);
+
+	assert((access_type & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM);
+
+	switch (access_type & IORESOURCE_BITS) {
+	case IORESOURCE_MEM_32BIT:
+		/* TODO: handle endianness */
+		return readl(reg);
+
+	case IORESOURCE_MEM_8BIT:
+		return readb(reg);
+	}
+
+	assert(0);
+	return 0;
+}
+
+static inline void write_reg(struct NS16550 *port, unsigned idx, u8 value)
+{
+	unsigned access_type;
+	void *reg;
+
+	access_type = get_access_type(port);
+	reg = get_reg_addr(port, idx, access_type);
+
+	if ((access_type & IORESOURCE_TYPE_BITS) == IORESOURCE_IO) {
+		outb(value, (ulong)reg);
+		return;
+	}
+
+	switch (access_type & IORESOURCE_BITS) {
+	case IORESOURCE_MEM_32BIT:
+		/* TODO: handle endianness */
+		writel(value, reg);
+		return;
+
+	case IORESOURCE_MEM_8BIT:
+		writeb(value, reg);
+		return;
+	}
+
+	assert(0);
+}
+
+#define GEN_ACCESSORS(idx, name)				\
+static inline u8 read_##name(struct NS16550 *port)		\
+{								\
+	return read_reg(port, idx);				\
+}								\
+								\
+static inline void write_##name(struct NS16550 *port, u8 value)	\
+{								\
+	write_reg(port, idx, value);				\
+}
+
+GEN_ACCESSORS(0x0, rbr)
+GEN_ACCESSORS(0x0, thr)
+GEN_ACCESSORS(0x0, dll)
+GEN_ACCESSORS(0x1, ier)
+GEN_ACCESSORS(0x1, dlm)
+GEN_ACCESSORS(0x2, fcr)
+GEN_ACCESSORS(0x2, iir)
+GEN_ACCESSORS(0x3, lcr)
+GEN_ACCESSORS(0x4, mcr)
+GEN_ACCESSORS(0x5, lsr)
+GEN_ACCESSORS(0x8, mdr1)
+GEN_ACCESSORS(0xc, regC)
+
 #define UART_LCRVAL UART_LCR_8N1		/* 8 data, 1 stop, no parity */
 #define UART_MCRVAL (UART_MCR_DTR | \
 		     UART_MCR_RTS)		/* RTS/DTR */
@@ -23,22 +154,6 @@ DECLARE_GLOBAL_DATA_PTR;
 		     UART_FCR_RXSR |	\
 		     UART_FCR_TXSR)		/* Clear & enable FIFOs */
 
-#ifndef CONFIG_DM_SERIAL
-#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
-#define serial_out(x, y)	outb(x, (ulong)y)
-#define serial_in(y)		inb((ulong)y)
-#elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE > 0)
-#define serial_out(x, y)	out_be32(y, x)
-#define serial_in(y)		in_be32(y)
-#elif defined(CONFIG_SYS_NS16550_MEM32) && (CONFIG_SYS_NS16550_REG_SIZE < 0)
-#define serial_out(x, y)	out_le32(y, x)
-#define serial_in(y)		in_le32(y)
-#else
-#define serial_out(x, y)	writeb(x, y)
-#define serial_in(y)		readb(y)
-#endif
-#endif /* !CONFIG_DM_SERIAL */
-
 #if defined(CONFIG_SOC_KEYSTONE)
 #define UART_REG_VAL_PWREMU_MGMT_UART_DISABLE   0
 #define UART_REG_VAL_PWREMU_MGMT_UART_ENABLE ((1 << 14) | (1 << 13) | (1 << 0))
@@ -60,72 +175,6 @@ DECLARE_GLOBAL_DATA_PTR;
 #define CONFIG_SYS_NS16550_CLK  0
 #endif
 
-static inline void serial_out_shift(void *addr, int shift, int value)
-{
-#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
-	outb(value, (ulong)addr);
-#elif defined(CONFIG_SYS_NS16550_MEM32) && !defined(CONFIG_SYS_BIG_ENDIAN)
-	out_le32(addr, value);
-#elif defined(CONFIG_SYS_NS16550_MEM32) && defined(CONFIG_SYS_BIG_ENDIAN)
-	out_be32(addr, value);
-#elif defined(CONFIG_SYS_NS16550_MEM32)
-	writel(value, addr);
-#elif defined(CONFIG_SYS_BIG_ENDIAN)
-	writeb(value, addr + (1 << shift) - 1);
-#else
-	writeb(value, addr);
-#endif
-}
-
-static inline int serial_in_shift(void *addr, int shift)
-{
-#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
-	return inb((ulong)addr);
-#elif defined(CONFIG_SYS_NS16550_MEM32) && !defined(CONFIG_SYS_BIG_ENDIAN)
-	return in_le32(addr);
-#elif defined(CONFIG_SYS_NS16550_MEM32) && defined(CONFIG_SYS_BIG_ENDIAN)
-	return in_be32(addr);
-#elif defined(CONFIG_SYS_NS16550_MEM32)
-	return readl(addr);
-#elif defined(CONFIG_SYS_BIG_ENDIAN)
-	return readb(addr + (1 << shift) - 1);
-#else
-	return readb(addr);
-#endif
-}
-
-static void ns16550_writeb(NS16550_t port, int offset, int value)
-{
-	struct ns16550_platdata *plat = port->plat;
-	unsigned char *addr;
-
-	offset *= 1 << plat->reg_shift;
-	addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
-	/*
-	 * As far as we know it doesn't make sense to support selection of
-	 * these options at run-time, so use the existing CONFIG options.
-	 */
-	serial_out_shift(addr, plat->reg_shift, value);
-}
-
-static int ns16550_readb(NS16550_t port, int offset)
-{
-	struct ns16550_platdata *plat = port->plat;
-	unsigned char *addr;
-
-	offset *= 1 << plat->reg_shift;
-	addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
-
-	return serial_in_shift(addr, plat->reg_shift);
-}
-
-/* We can clean these up once everything is moved to driver model */
-#define serial_out(value, addr)	\
-	ns16550_writeb(com_port, \
-		(unsigned char *)addr - (unsigned char *)com_port, value)
-#define serial_in(addr) \
-	ns16550_readb(com_port, \
-		(unsigned char *)addr - (unsigned char *)com_port)
 #endif
 
 static inline int calc_divisor(NS16550_t port, int clock, int baudrate)
@@ -151,10 +200,10 @@ int ns16550_calc_divisor(NS16550_t port, int clock, int baudrate)
 
 static void NS16550_setbrg(NS16550_t com_port, int baud_divisor)
 {
-	serial_out(UART_LCR_BKSE | UART_LCRVAL, &com_port->lcr);
-	serial_out(baud_divisor & 0xff, &com_port->dll);
-	serial_out((baud_divisor >> 8) & 0xff, &com_port->dlm);
-	serial_out(UART_LCRVAL, &com_port->lcr);
+	write_lcr(com_port, UART_LCR_BKSE | UART_LCRVAL);
+	write_dll(com_port, baud_divisor & 0xff);
+	write_dlm(com_port, (baud_divisor >> 8) & 0xff);
+	write_lcr(com_port, UART_LCRVAL);
 }
 
 void NS16550_init(NS16550_t com_port, int baud_divisor)
@@ -166,24 +215,24 @@ void NS16550_init(NS16550_t com_port, int baud_divisor)
 	 * before SPL starts only THRE bit is set. We have to empty the
 	 * transmitter before initialization starts.
 	 */
-	if ((serial_in(&com_port->lsr) & (UART_LSR_TEMT | UART_LSR_THRE))
+	if ((read_lsr(com_port) & (UART_LSR_TEMT | UART_LSR_THRE))
 	     == UART_LSR_THRE) {
 		if (baud_divisor != -1)
 			NS16550_setbrg(com_port, baud_divisor);
-		serial_out(0, &com_port->mdr1);
+		write_mdr1(com_port, 0);
 	}
 #endif
 
-	while (!(serial_in(&com_port->lsr) & UART_LSR_TEMT))
+	while (!(read_lsr(com_port) & UART_LSR_TEMT))
 		;
 
-	serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
+	write_ier(com_port, CONFIG_SYS_NS16550_IER);
 #if defined(CONFIG_OMAP) || defined(CONFIG_AM33XX) || \
 			defined(CONFIG_TI81XX) || defined(CONFIG_AM43XX)
-	serial_out(0x7, &com_port->mdr1);	/* mode select reset TL16C750*/
+	write_mdr1(com_port, 0x7);	/* mode select reset TL16C750*/
 #endif
-	serial_out(UART_MCRVAL, &com_port->mcr);
-	serial_out(UART_FCRVAL, &com_port->fcr);
+	write_mcr(com_port, UART_MCRVAL);
+	write_fcr(com_port, UART_FCRVAL);
 	if (baud_divisor != -1)
 		NS16550_setbrg(com_port, baud_divisor);
 #if defined(CONFIG_OMAP) || \
@@ -191,29 +240,29 @@ void NS16550_init(NS16550_t com_port, int baud_divisor)
 	defined(CONFIG_TI81XX) || defined(CONFIG_AM43XX)
 
 	/* /16 is proper to hit 115200 with 48MHz */
-	serial_out(0, &com_port->mdr1);
+	write_mdr1(com_port, 0);
 #endif /* CONFIG_OMAP */
 #if defined(CONFIG_SOC_KEYSTONE)
-	serial_out(UART_REG_VAL_PWREMU_MGMT_UART_ENABLE, &com_port->regC);
+	write_regC(com_port, UART_REG_VAL_PWREMU_MGMT_UART_ENABLE);
 #endif
 }
 
 #ifndef CONFIG_NS16550_MIN_FUNCTIONS
 void NS16550_reinit(NS16550_t com_port, int baud_divisor)
 {
-	serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
+	write_ier(com_port, CONFIG_SYS_NS16550_IER);
 	NS16550_setbrg(com_port, 0);
-	serial_out(UART_MCRVAL, &com_port->mcr);
-	serial_out(UART_FCRVAL, &com_port->fcr);
+	write_mcr(com_port, UART_MCRVAL);
+	write_fcr(com_port, UART_FCRVAL);
 	NS16550_setbrg(com_port, baud_divisor);
 }
 #endif /* CONFIG_NS16550_MIN_FUNCTIONS */
 
 void NS16550_putc(NS16550_t com_port, char c)
 {
-	while ((serial_in(&com_port->lsr) & UART_LSR_THRE) == 0)
+	while ((read_lsr(com_port) & UART_LSR_THRE) == 0)
 		;
-	serial_out(c, &com_port->thr);
+	write_thr(com_port, c);
 
 	/*
 	 * Call watchdog_reset() upon newline. This is done here in putc
@@ -228,19 +277,19 @@ void NS16550_putc(NS16550_t com_port, char c)
 #ifndef CONFIG_NS16550_MIN_FUNCTIONS
 char NS16550_getc(NS16550_t com_port)
 {
-	while ((serial_in(&com_port->lsr) & UART_LSR_DR) == 0) {
+	while ((read_lsr(com_port) & UART_LSR_DR) == 0) {
 #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_USB_TTY)
 		extern void usbtty_poll(void);
 		usbtty_poll();
 #endif
 		WATCHDOG_RESET();
 	}
-	return serial_in(&com_port->rbr);
+	return read_rbr(com_port);
 }
 
 int NS16550_tstc(NS16550_t com_port)
 {
-	return (serial_in(&com_port->lsr) & UART_LSR_DR) != 0;
+	return (read_lsr(com_port) & UART_LSR_DR) != 0;
 }
 
 #endif /* CONFIG_NS16550_MIN_FUNCTIONS */
@@ -249,6 +298,40 @@ int NS16550_tstc(NS16550_t com_port)
 
 #include <debug_uart.h>
 
+static inline void serial_out_shift(void *addr, int shift, int value)
+{
+#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
+	outb(value, (ulong)addr);
+#elif defined(CONFIG_SYS_NS16550_MEM32) && !defined(CONFIG_SYS_BIG_ENDIAN)
+	out_le32(addr, value);
+#elif defined(CONFIG_SYS_NS16550_MEM32) && defined(CONFIG_SYS_BIG_ENDIAN)
+	out_be32(addr, value);
+#elif defined(CONFIG_SYS_NS16550_MEM32)
+	writel(value, addr);
+#elif defined(CONFIG_SYS_BIG_ENDIAN)
+	writeb(value, addr + (1 << shift) - 1);
+#else
+	writeb(value, addr);
+#endif
+}
+
+static inline int serial_in_shift(void *addr, int shift)
+{
+#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
+	return inb((ulong)addr);
+#elif defined(CONFIG_SYS_NS16550_MEM32) && !defined(CONFIG_SYS_BIG_ENDIAN)
+	return in_le32(addr);
+#elif defined(CONFIG_SYS_NS16550_MEM32) && defined(CONFIG_SYS_BIG_ENDIAN)
+	return in_be32(addr);
+#elif defined(CONFIG_SYS_NS16550_MEM32)
+	return readl(addr);
+#elif defined(CONFIG_SYS_BIG_ENDIAN)
+	return readb(addr + (1 << shift) - 1);
+#else
+	return readb(addr);
+#endif
+}
+
 #define serial_dout(reg, value)	\
 	serial_out_shift((char *)com_port + \
 		((char *)reg - (char *)com_port) * \
@@ -301,9 +384,9 @@ static int ns16550_serial_putc(struct udevice *dev, const char ch)
 {
 	struct NS16550 *const com_port = dev_get_priv(dev);
 
-	if (!(serial_in(&com_port->lsr) & UART_LSR_THRE))
+	if (!(read_lsr(com_port) & UART_LSR_THRE))
 		return -EAGAIN;
-	serial_out(ch, &com_port->thr);
+	write_thr(com_port, ch);
 
 	/*
 	 * Call watchdog_reset() upon newline. This is done here in putc
@@ -322,19 +405,19 @@ static int ns16550_serial_pending(struct udevice *dev, bool input)
 	struct NS16550 *const com_port = dev_get_priv(dev);
 
 	if (input)
-		return serial_in(&com_port->lsr) & UART_LSR_DR ? 1 : 0;
+		return read_lsr(com_port) & UART_LSR_DR ? 1 : 0;
 	else
-		return serial_in(&com_port->lsr) & UART_LSR_THRE ? 0 : 1;
+		return read_lsr(com_port) & UART_LSR_THRE ? 0 : 1;
 }
 
 static int ns16550_serial_getc(struct udevice *dev)
 {
 	struct NS16550 *const com_port = dev_get_priv(dev);
 
-	if (!(serial_in(&com_port->lsr) & UART_LSR_DR))
+	if (!(read_lsr(com_port) & UART_LSR_DR))
 		return -EAGAIN;
 
-	return serial_in(&com_port->rbr);
+	return read_rbr(com_port);
 }
 
 static int ns16550_serial_setbrg(struct udevice *dev, int baudrate)
@@ -367,7 +450,7 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
 	fdt_addr_t addr;
 
 	/* try Processor Local Bus device first */
-	addr = dev_get_addr(dev);
+	addr = dev_get_addr_flags(dev, &plat->access_flags);
 #if defined(CONFIG_PCI) && defined(CONFIG_DM_PCI)
 	if (addr == FDT_ADDR_T_NONE) {
 		/* then try pci device */
@@ -394,12 +477,22 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
 			return ret;
 
 		addr = bar;
+		plat->access_flags = IORESOURCE_MEM;
 	}
 #endif
 
 	if (addr == FDT_ADDR_T_NONE)
 		return -EINVAL;
 
+	if (((plat->access_flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM) &&
+		!(plat->access_flags & IORESOURCE_BITS)) {
+		/* determine the access width from config macros */
+		if (config_enabled(CONFIG_SYS_NS16550_MEM32))
+			plat->access_flags |= IORESOURCE_MEM_32BIT;
+		else
+			plat->access_flags |= IORESOURCE_MEM_8BIT;
+	}
+
 	plat->base = addr;
 	plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
 					 "reg-shift", 0);
diff --git a/include/ns16550.h b/include/ns16550.h
index 4e62067..a9e9dc2 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -51,11 +51,17 @@
  * @base:		Base register address
  * @reg_shift:		Shift size of registers (0=byte, 1=16bit, 2=32bit...)
  * @clock:		UART base clock speed in Hz
+ * @access_flags:	IORESOURCE flags indicating the type of access to
+ * 			perform to the UART registers, for example
+ * 			%IORESOURCE_IO to indicate I/O port access,
+ * 			%IORESOURCE_MEM_32BIT for 32 bit memory mapped I/O
+ * 			etc.
  */
 struct ns16550_platdata {
 	unsigned long base;
 	int reg_shift;
 	int clock;
+	unsigned int access_flags;
 };
 
 struct udevice;
@@ -90,6 +96,9 @@ struct NS16550 {
 #endif
 #ifdef CONFIG_DM_SERIAL
 	struct ns16550_platdata *plat;
+# define ns16550_plat(port)	((port)->plat)
+#else
+# define ns16550_plat(port)	NULL
 #endif
 };
 
-- 
2.7.0



More information about the U-Boot mailing list