[PATCH v2 1/4] serial: ns16550: Support run-time configuration

Park, Aiden aiden.park at intel.com
Tue Dec 10 20:35:23 CET 2019


Hi Simon,

> -----Original Message-----
> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Simon Glass
> Sent: Monday, December 9, 2019 8:59 AM
> To: U-Boot Mailing List <u-boot at lists.denx.de>
> Cc: Stefan Roese <sr at denx.de>; Angelo Dureghello <angelo at sysam.it>
> Subject: [PATCH v2 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 <sjg at chromium.org>
> ---
> 
> 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 | 57 ++++++++++++++++++++++++++++++++++----
> --
>  include/ns16550.h        | 13 +++++++++
>  3 files changed, 83 insertions(+), 8 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..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);
> +	}
> +}
IO needs to use outb(). It breaks QEMU 0x3f8 IO (reg_width = 1, flags=IO).
I have verified 0x3f8 IO on QEMU and MMIO32 on APL with below code.
	if (plat->flags & NS16550_FLAG_IO) {
		outb(value, addr);
	} else {
		if (plat->flags & NS16550_FLAG_BE) {
			if (plat->reg_width == 1)
				writeb(value, addr + (1 << plat->reg_shift) - 1);
			else
				out_be32(addr, value);
		} else {
			if (plat->reg_width == 1)
				writeb(value, addr);
			else
				out_le32(addr, value);
		}
	}

> +
> +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);
> +	}
> +}
Similar as above.
	if (plat->flags & NS16550_FLAG_IO) {
		return inb((ulong)addr);
	} else {
		if (plat->flags & NS16550_FLAG_BE) {
			if (plat->reg_width == 1)
				return readb(addr + (1 << plat->reg_shift) - 1);
			else
				return in_be32(addr);
		} else {
			if (plat->reg_width == 1)
				return readb(addr);
			else
				return in_le32(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
> --
> 2.24.0.393.g34dc348eaf-goog

Best Regards,
Aiden


More information about the U-Boot mailing list