Hi Bin/Simon,
Thanks for adding me in this review thread. I like this approach very much. Let me make a patch for Slim Bootloader to follow up this dynamic ns16550 and send it for review. Thanks.
-----Original Message----- From: Bin Meng bmeng.cn@gmail.com Sent: Sunday, December 8, 2019 3:31 AM To: Simon Glass sjg@chromium.org; Park, Aiden aiden.park@intel.com Cc: U-Boot Mailing List u-boot@lists.denx.de Subject: Re: [PATCH 1/4] serial: n16550: Support run-time configuration
+Aiden
Hi Simon,
On Fri, Dec 6, 2019 at 7:04 AM Simon Glass sjg@chromium.org 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 sjg@chromium.org
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_COREBOOTI believe we should also turn it on for slimbootloader.
helpEnable 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 isused for
access. If not enabled, then the UART is memory-mapped.CONFIG_SYS_NS16550_MEM32 - if memory-mapped, indicates that32-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 ofUARTs in a system. This option avoids this problem at the cost of aslightly 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);elsewritel(value, addr);} else {if (plat->reg_width == 1)writeb(value, addr);else if (plat->flags & NS16550_FLAG_IO)out_le32(addr, value);elsewritel(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);elsereturn readl(addr);} else {if (plat->reg_width == 1)return readb(addr);else if (plat->flags & NS16550_FLAG_IO)return in_le32(addr);elsereturn 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);elseserial_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);elsereturn 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
Best Regards, Aiden