+Aiden Hi Simon,
On Fri, Dec 6, 2019 at 7:04 AM Simon Glass <[email protected]> wrote: > > At present this driver uses an assortment of CONFIG options to control > how it accesses the hardware. This is painful for platforms that are > supposed to be controlled by a device tree or a previous-stage bootloader. > > Add a new CONFIG option to enable fully dynamic configuration. This > controls register spacing, size, offset and endianness. > > Signed-off-by: Simon Glass <[email protected]> > --- > > drivers/serial/Kconfig | 20 ++++++++++++++ > drivers/serial/ns16550.c | 57 ++++++++++++++++++++++++++++++++++------ > include/ns16550.h | 13 +++++++++ > 3 files changed, 82 insertions(+), 8 deletions(-) > > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig > index d36a0108ea..50710ab998 100644 > --- a/drivers/serial/Kconfig > +++ b/drivers/serial/Kconfig > @@ -598,6 +598,26 @@ config SYS_NS16550 > be used. It can be a constant or a function to get clock, eg, > get_serial_clock(). > > +config NS16550_DYNAMIC > + bool "Allow NS16550 to be configured at runtime" nits: run-time > + default y if SYS_COREBOOT I believe we should also turn it on for slimbootloader. > + help > + Enable this option to allow device-tree control of the driver. > + > + Normally this driver is controlled by the following options: > + > + CONFIG_SYS_NS16550_PORT_MAPPED - indicates that port I/O is used for > + access. If not enabled, then the UART is memory-mapped. > + CONFIG_SYS_NS16550_MEM32 - if memory-mapped, indicates that 32-bit > + access should be used (instead of 8-bit) > + CONFIG_SYS_NS16550_REG_SIZE - indicates endianness. If positive, This is not for endianness, but for the register width. > + big-endian access is used. If negative, little-endian is used. > + > + It is not good practive for a driver to be statically configured, not a good practice > + since it prevents the same driver being used for different types of > + UARTs in a system. This option avoids this problem at the cost of a > + slightly increased code size. > + > config INTEL_MID_SERIAL > bool "Intel MID platform UART support" > depends on DM_SERIAL && OF_CONTROL > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c > index 754b6e9921..96c4471efd 100644 > --- a/drivers/serial/ns16550.c > +++ b/drivers/serial/ns16550.c > @@ -92,19 +92,57 @@ static inline int serial_in_shift(void *addr, int shift) > #define CONFIG_SYS_NS16550_CLK 0 > #endif > > +static void serial_out_dynamic(struct ns16550_platdata *plat, u8 *addr, > + int value) > +{ > + if (plat->flags & NS16550_FLAG_BE) { > + if (plat->reg_width == 1) > + writeb(value, addr + (1 << plat->reg_shift) - 1); > + else if (plat->flags & NS16550_FLAG_IO) > + out_be32(addr, value); > + else > + writel(value, addr); > + } else { > + if (plat->reg_width == 1) > + writeb(value, addr); > + else if (plat->flags & NS16550_FLAG_IO) > + out_le32(addr, value); > + else > + writel(value, addr); > + } > +} > + > +static int serial_in_dynamic(struct ns16550_platdata *plat, u8 *addr) > +{ > + if (plat->flags & NS16550_FLAG_BE) { > + if (plat->reg_width == 1) > + return readb(addr + (1 << plat->reg_shift) - 1); > + else if (plat->flags & NS16550_FLAG_IO) > + return in_be32(addr); > + else > + return readl(addr); > + } else { > + if (plat->reg_width == 1) > + return readb(addr); > + else if (plat->flags & NS16550_FLAG_IO) > + return in_le32(addr); > + else > + return readl(addr); > + } > +} > + > 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 = (unsigned char *)plat->base + offset; > + addr = (unsigned char *)plat->base + offset + plat->reg_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_offset, plat->reg_shift, value); > + if (IS_ENABLED(CONFIG_NS16550_DYNAMIC)) > + serial_out_dynamic(plat, addr, value); > + else > + serial_out_shift(addr, plat->reg_shift, value); > } > > static int ns16550_readb(NS16550_t port, int offset) > @@ -113,9 +151,12 @@ static int ns16550_readb(NS16550_t port, int offset) > unsigned char *addr; > > offset *= 1 << plat->reg_shift; > - addr = (unsigned char *)plat->base + offset; > + addr = (unsigned char *)plat->base + offset + plat->reg_offset; > > - return serial_in_shift(addr + plat->reg_offset, plat->reg_shift); > + if (IS_ENABLED(CONFIG_NS16550_DYNAMIC)) > + return serial_in_dynamic(plat, addr); > + else > + return serial_in_shift(addr, plat->reg_shift); > } > > static u32 ns16550_getfcr(NS16550_t port) > diff --git a/include/ns16550.h b/include/ns16550.h > index 701efeea85..ba9b71962d 100644 > --- a/include/ns16550.h > +++ b/include/ns16550.h > @@ -31,6 +31,9 @@ > #define CONFIG_SYS_NS16550_REG_SIZE (-1) > #endif > > +#ifdef CONFIG_NS16550_DYNAMIC > +#define UART_REG(x) unsigned char x > +#else > #if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == > 0) > #error "Please define NS16550 registers size." > #elif defined(CONFIG_SYS_NS16550_MEM32) && !defined(CONFIG_DM_SERIAL) > @@ -44,6 +47,12 @@ > unsigned char x; \ > unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1]; > #endif > +#endif /* CONFIG_NS16550_DYNAMIC */ > + > +enum ns16550_flags { > + NS16550_FLAG_IO = 1 << 0, /* Use I/O access (else mem-mapped) */ > + NS16550_FLAG_BE = 1 << 1, /* Use big-endian access (else little) */ > +}; > > /** > * struct ns16550_platdata - information about a NS16550 port > @@ -51,7 +60,10 @@ > * @base: Base register address > * @reg_width: IO accesses size of registers (in bytes) > * @reg_shift: Shift size of registers (0=byte, 1=16bit, 2=32bit...) > + * @reg_offset: Offset to start of registers > * @clock: UART base clock speed in Hz > + * @fcr: Offset of FCR register (normally UART_FCR_DEFVAL) > + * @flags: A few flags (enum ns16550_flags) > * @bdf: PCI slot/function (pci_dev_t) > */ > struct ns16550_platdata { > @@ -61,6 +73,7 @@ struct ns16550_platdata { > int reg_offset; > int clock; > u32 fcr; > + int flags; > #if defined(CONFIG_PCI) && defined(CONFIG_SPL) > int bdf; > #endif > -- Regards, Bin
