Hi Simon,
Thanks for enabling this dynamic serial config. I have verified this with Slim
Bootloader
change(https://patchwork.ozlabs.org/project/uboot/list/?series=149214) on QEMU
and APL platform, and it works well as expected.
Reviewed-by: Aiden Park <aiden.p...@intel.com>
Tested-by: Aiden Park <aiden.p...@intel.com>
> -----Original Message-----
> From: U-Boot <u-boot-boun...@lists.denx.de> On Behalf Of Simon Glass
> Sent: Thursday, December 19, 2019 4:58 PM
> To: U-Boot Mailing List <u-boot@lists.denx.de>
> Cc: Stefan Roese <s...@denx.de>; Angelo Dureghello <ang...@sysam.it>
> Subject: [PATCH v3 1/4] serial: ns16550: Support run-time configuration
>
> 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 <s...@chromium.org>
> ---
>
> Changes in v3:
> - Rewrite the conditional logic to a fix a bug and match serial_xx_shift()
> - Add a separate flag for whether to use endian-aware out() functions
>
> Changes in v2:
> - runtime -> run-time
> - Enable run-time config for slimbootloader too
> - Improve Kconfig help based on Bin's comments
> - Use ns16550 in patch subject
>
> drivers/serial/Kconfig | 21 ++++++++++++++
> drivers/serial/ns16550.c | 59 ++++++++++++++++++++++++++++++++++----
> --
> include/ns16550.h | 16 ++++++++++-
> 3 files changed, 87 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig index
> ece7d87d4c..472a9f0929 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -598,6 +598,27 @@ 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"
> + default y if SYS_COREBOOT || SYS_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 register width and also
> + endianness. If positive, big-endian access is used. If negative,
> + little-endian is used.
> +
> + It is not a good practice for a driver to be statically configured,
> + 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..eacca5b703 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -92,19 +92,59 @@ 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_IO) {
> + outb(value, addr);
> + } else if (plat->reg_width == 4) {
> + if (plat->flags & NS16550_FLAG_ENDIAN) {
> + if (plat->flags & NS16550_FLAG_BE)
> + out_be32(addr, value);
> + else
> + out_le32(addr, value);
> + } else {
> + writel(value, addr);
> + }
> + } else if (plat->flags & NS16550_FLAG_BE) {
> + writeb(value, addr + (1 << plat->reg_shift) - 1);
> + } else {
> + writeb(value, addr);
> + }
> +}
> +
> +static int serial_in_dynamic(struct ns16550_platdata *plat, u8 *addr) {
> + if (plat->flags & NS16550_FLAG_IO) {
> + return inb(addr);
> + } else if (plat->reg_width == 4) {
> + if (plat->flags & NS16550_FLAG_ENDIAN) {
> + if (plat->flags & NS16550_FLAG_BE)
> + return in_be32(addr);
> + else
> + return in_le32(addr);
> + } else {
> + return readl(addr);
> + }
> + } else if (plat->flags & NS16550_FLAG_BE) {
> + return readb(addr + (1 << plat->reg_shift) - 1);
> + } else {
> + return readb(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 +153,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..18c9077755 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,14 +47,24 @@
> 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_ENDIAN = 1 << 1, /* Use out_le/be_32() */
> + NS16550_FLAG_BE = 1 << 2, /* Big-endian access (else
> little) */
> +};
>
> /**
> * struct ns16550_platdata - information about a NS16550 port
> *
> * @base: Base register address
> - * @reg_width: IO accesses size of registers (in bytes)
> + * @reg_width: IO accesses size of registers (in bytes, 1 or 4)
> * @reg_shift: Shift size of registers (0=byte, 1=16bit,
> 2=32bit...)
> + * @reg_offset: Offset to start of registers (normally 0)
> * @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 +74,7 @@ struct ns16550_platdata {
> int reg_offset;
> int clock;
> u32 fcr;
> + int flags;
> #if defined(CONFIG_PCI) && defined(CONFIG_SPL)
> int bdf;
> #endif
> --
> 2.24.1.735.g03f4e72817-goog
Best Regards,
Aiden
