[U-Boot] [PATCH] ns16550: Clean up & unify register accessors
Simon Glass
sjg at chromium.org
Thu Feb 4 05:17:00 CET 2016
Hi Paul,
On 2 February 2016 at 07:12, Paul Burton <paul.burton at imgtec.com> wrote:
> 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!".
Digging is good, you are brave. I have some ideas on where this should
go - see below...
> ---
> 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);
Do you think these two could be called by the caller of write_reg()
and passed in as parameters? Then it will be possible to call it
without platform data.
> +
> + 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)
Ick, why do we need all this? How about an enum for the register
number, instead? I.e. make the register number a parameter.
> +
> #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
>
Regards,
Simon
More information about the U-Boot
mailing list