[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@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); + } +} + +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
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 Reviewed-by: Bin Meng bmeng.cn@gmail.com ---
Changes in v2: None
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
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 Reviewed-by: Bin Meng bmeng.cn@gmail.com ---
Changes in v2: None
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 472a9f0929..c403c2fc25 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, +};
Use this UART to improve the compatibility of U-Boot when used as a coreboot payload.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Bin Meng bmeng.cn@gmail.com ---
Changes in v2: None
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"; };
Am 09.12.2019 um 17:59 schrieb 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
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 foraccess. If not enabled, then the UART is memory-mapped.CONFIG_SYS_NS16550_MEM32 - if memory-mapped, indicates that 32-bitaccess should be used (instead of 8-bit)CONFIG_SYS_NS16550_REG_SIZE - indicates register width and alsoendianness. 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 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);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)
*/ struct ns16550_platdata {
- @flags: A few flags (enum ns16550_flags)
- @bdf: PCI slot/function (pci_dev_t)
@@ -61,6 +73,7 @@ struct ns16550_platdata { int reg_offset; int clock; u32 fcr;
- int flags;
I'm not too fond of adding 4 bytes here for nothing on most platforms, but unless you come up with a better idea than cluttering this file with ifdefs:
Reviewed-by: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com
#if defined(CONFIG_PCI) && defined(CONFIG_SPL) int bdf; #endif
On Tue, Dec 10, 2019 at 12:59 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
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(-)
Reviewed-by: Bin Meng bmeng.cn@gmail.com
Hi Simon,
-----Original Message----- From: U-Boot u-boot-bounces@lists.denx.de On Behalf Of Simon Glass Sent: Monday, December 9, 2019 8:59 AM To: U-Boot Mailing List u-boot@lists.denx.de Cc: Stefan Roese sr@denx.de; Angelo Dureghello angelo@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@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 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 register width and alsoendianness. 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 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);- }
+}
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);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);- }
+}
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
Hi Simon,
On Wed, Dec 11, 2019 at 3:35 AM Park, Aiden aiden.park@intel.com wrote:
Hi Simon,
-----Original Message----- From: U-Boot u-boot-bounces@lists.denx.de On Behalf Of Simon Glass Sent: Monday, December 9, 2019 8:59 AM To: U-Boot Mailing List u-boot@lists.denx.de Cc: Stefan Roese sr@denx.de; Angelo Dureghello angelo@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@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_SLIMBOOTLOADERhelpEnable 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 register width and alsoendianness. 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 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);}+}
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); } }
Would you post a v3 that fixes the issue that Aiden pointed out?
Regards, Bin
Hi Bin,
On Mon, 16 Dec 2019 at 00:03, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Wed, Dec 11, 2019 at 3:35 AM Park, Aiden aiden.park@intel.com wrote:
Hi Simon,
-----Original Message----- From: U-Boot u-boot-bounces@lists.denx.de On Behalf Of Simon Glass Sent: Monday, December 9, 2019 8:59 AM To: U-Boot Mailing List u-boot@lists.denx.de Cc: Stefan Roese sr@denx.de; Angelo Dureghello angelo@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@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_SLIMBOOTLOADERhelpEnable 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 register width and alsoendianness. 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 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);}+}
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); } }
Would you post a v3 that fixes the issue that Aiden pointed out?
Yes, but actually I have found that we need one more case, so it needs a bit more effort. Will post an update series by Monday.
Regards, SImon
participants (4)
-
Bin Meng -
Park, Aiden -
Simon Glass -
Simon Goldschmidt