From: Paul Burton Date: Tue, 2 Feb 2016 14:14:32 +0000 Subject: [U-Boot] [RFC PATCH] ns16550: Clean up & unify register accessors In-Reply-To: <1454422344-27462-1-git-send-email-paul.burton@imgtec.com> References: <1454422344-27462-1-git-send-email-paul.burton@imgtec.com> Message-ID: <20160202141432.GA30470@NP-P-BURTON> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, Feb 02, 2016 at 02:12:24PM +0000, Paul Burton 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 Sorry, this should really have had "RFC" in the subject... Thanks, Paul > Cc: Simon Glass > Signed-off-by: Paul Burton > --- > 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 > #include > #include > +#include > +#include > #include > #include > > 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 > > +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 >