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

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

[Simon Glass]

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"
+	default y if SYS_COREBOOT
+	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,
+	     big-endian access is used. If negative, little-endian is used.
+
+	  It is not good practive 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);
+	}
+}
+
+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
-- 
2.24.0.393.g34dc348eaf-goog

[PATCH 2/4] x86: Update coreboot serial table struct

[Simon Glass]

Since mid 2016, coreboot has additional fields in the serial struct that
it passes down to U-Boot. Add these so we are in sync.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/include/asm/coreboot_tables.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/x86/include/asm/coreboot_tables.h b/arch/x86/include/asm/coreboot_tables.h
index 2c54e24e02..61de0077d7 100644
--- a/arch/x86/include/asm/coreboot_tables.h
+++ b/arch/x86/include/asm/coreboot_tables.h
@@ -97,6 +97,25 @@ struct cb_serial {
 	u32 type;
 	u32 baseaddr;
 	u32 baud;
+	u32 regwidth;
+
+	/*
+	 * Crystal or input frequency to the chip containing the UART.
+	 * Provide the board specific details to allow the payload to
+	 * initialize the chip containing the UART and make independent
+	 * decisions as to which dividers to select and their values
+	 * to eventually arrive at the desired console baud-rate.
+	 */
+	u32 input_hertz;
+
+	/*
+	 * UART PCI address: bus, device, function
+	 * 1 << 31 - Valid bit, PCI UART in use
+	 * Bus << 20
+	 * Device << 15
+	 * Function << 12
+	 */
+	u32 uart_pci_addr;
 };
 
 #define CB_TAG_CONSOLE			0x0010
-- 
2.24.0.393.g34dc348eaf-goog

[PATCH 3/4] x86: serial: Add a coreboot serial driver

[Simon Glass]

Coreboot can provide information about the serial device in use on a
platform. Add a driver that uses this information to produce a working
UART.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/serial/Kconfig           | 11 ++++++++
 drivers/serial/Makefile          |  1 +
 drivers/serial/serial_coreboot.c | 46 ++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)
 create mode 100644 drivers/serial/serial_coreboot.c

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 50710ab998..035ff08059 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -539,6 +539,17 @@ config BCM6345_SERIAL
 	help
 	  Select this to enable UART on BCM6345 SoCs.
 
+config COREBOOT_SERIAL
+	bool "Coreboot UART support"
+	depends on DM_SERIAL
+	default y if SYS_COREBOOT
+	select SYS_NS16550
+	help
+	  Select this to enable a ns16550-style UART where the platform data
+	  comes from the coreboot 'sysinfo' tables. This allows U-Boot to have
+	  a serial console on any platform without needing to change the
+	  device tree, etc.
+
 config FSL_LINFLEXUART
 	bool "Freescale Linflex UART support"
 	depends on DM_SERIAL
diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
index 06ee30697d..76b1811510 100644
--- a/drivers/serial/Makefile
+++ b/drivers/serial/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_AR933X_UART) += serial_ar933x.o
 obj-$(CONFIG_ARM_DCC) += arm_dcc.o
 obj-$(CONFIG_ATMEL_USART) += atmel_usart.o
 obj-$(CONFIG_BCM6345_SERIAL) += serial_bcm6345.o
+obj-$(CONFIG_COREBOOT_SERIAL) += serial_coreboot.o
 obj-$(CONFIG_EFI_APP) += serial_efi.o
 obj-$(CONFIG_LPC32XX_HSUART) += lpc32xx_hsuart.o
 obj-$(CONFIG_MCFUART) += mcfuart.o
diff --git a/drivers/serial/serial_coreboot.c b/drivers/serial/serial_coreboot.c
new file mode 100644
index 0000000000..ccab347514
--- /dev/null
+++ b/drivers/serial/serial_coreboot.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * UART support for U-Boot when launched from Coreboot
+ *
+ * Copyright 2019 Google LLC
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <ns16550.h>
+#include <serial.h>
+#include <asm/arch/sysinfo.h>
+
+static int coreboot_ofdata_to_platdata(struct udevice *dev)
+{
+	struct ns16550_platdata *plat = dev_get_platdata(dev);
+	struct cb_serial *cb_info = lib_sysinfo.serial;
+
+	plat->base = cb_info->baseaddr;
+	plat->reg_shift = cb_info->regwidth == 4 ? 2 : 0;
+	plat->reg_width = cb_info->regwidth;
+	plat->clock = cb_info->input_hertz;
+	plat->fcr = UART_FCR_DEFVAL;
+	plat->flags = 0;
+	if (cb_info->type == CB_SERIAL_TYPE_IO_MAPPED)
+		plat->flags |= NS16550_FLAG_IO;
+
+	return 0;
+}
+
+static const struct udevice_id coreboot_serial_ids[] = {
+	{ .compatible = "coreboot-serial" },
+	{ },
+};
+
+U_BOOT_DRIVER(coreboot_uart) = {
+	.name	= "coreboot_uart",
+	.id	= UCLASS_SERIAL,
+	.of_match	= coreboot_serial_ids,
+	.priv_auto_alloc_size = sizeof(struct NS16550),
+	.platdata_auto_alloc_size = sizeof(struct ns16550_platdata),
+	.ofdata_to_platdata  = coreboot_ofdata_to_platdata,
+	.probe	= ns16550_serial_probe,
+	.ops	= &ns16550_serial_ops,
+	.flags	= DM_FLAG_PRE_RELOC,
+};
-- 
2.24.0.393.g34dc348eaf-goog

[PATCH 4/4] x86: Move coreboot over to use the coreboot UART

[Simon Glass]

Use this UART to improve the compatibility of U-Boot when used as a
coreboot payload.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/dts/coreboot.dts | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/dts/coreboot.dts b/arch/x86/dts/coreboot.dts
index a88da6eafd..38ddaafa19 100644
--- a/arch/x86/dts/coreboot.dts
+++ b/arch/x86/dts/coreboot.dts
@@ -8,7 +8,6 @@
 /dts-v1/;
 
 /include/ "skeleton.dtsi"
-/include/ "serial.dtsi"
 /include/ "keyboard.dtsi"
 /include/ "pcspkr.dtsi"
 /include/ "reset.dtsi"
@@ -40,6 +39,11 @@
 		u-boot,dm-pre-reloc;
 	};
 
+	serial: serial {
+		u-boot,dm-pre-reloc;
+		compatible = "coreboot-serial";
+	};
+
 	coreboot-fb {
 		compatible = "coreboot-fb";
 	};
-- 
2.24.0.393.g34dc348eaf-goog

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

[Bin Meng]

+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_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

[PATCH 2/4] x86: Update coreboot serial table struct

[Bin Meng]

On Fri, Dec 6, 2019 at 7:04 AM Simon Glass <sjg@chromium.org> wrote:
>
> Since mid 2016, coreboot has additional fields in the serial struct that
> it passes down to U-Boot. Add these so we are in sync.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/include/asm/coreboot_tables.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 3/4] x86: serial: Add a coreboot serial driver

[Bin Meng]

On Fri, Dec 6, 2019 at 7:04 AM Simon Glass <sjg@chromium.org> wrote:
>
> Coreboot can provide information about the serial device in use on a
> platform. Add a driver that uses this information to produce a working
> UART.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  drivers/serial/Kconfig           | 11 ++++++++
>  drivers/serial/Makefile          |  1 +
>  drivers/serial/serial_coreboot.c | 46 ++++++++++++++++++++++++++++++++
>  3 files changed, 58 insertions(+)
>  create mode 100644 drivers/serial/serial_coreboot.c
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 4/4] x86: Move coreboot over to use the coreboot UART

[Bin Meng]

On Fri, Dec 6, 2019 at 7:04 AM Simon Glass <sjg@chromium.org> wrote:
>
> Use this UART to improve the compatibility of U-Boot when used as a
> coreboot payload.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/dts/coreboot.dts | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

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

[Park, Aiden]

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_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

Best Regards,
Aiden