| Message ID | 20191220005821.205184-1-sjg@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | [v3,1/4] serial: ns16550: Support run-time configuration | expand |
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.park@intel.com> Tested-by: Aiden Park <aiden.park@intel.com> > -----Original Message----- > From: U-Boot <u-boot-bounces@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 <sr@denx.de>; Angelo Dureghello <angelo@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 <sjg@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
On Fri, Dec 20, 2019 at 8:58 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 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(-) > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
On Mon, Feb 3, 2020 at 10:29 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Fri, Dec 20, 2019 at 8:58 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 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(-) > > > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> applied to u-boot-x86, thanks!
Hi Simon, On Mon, Feb 3, 2020 at 10:34 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Mon, Feb 3, 2020 at 10:29 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > On Fri, Dec 20, 2019 at 8:58 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 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(-) > > > > > > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > > applied to u-boot-x86, thanks! This patch unfortunately breaks lots of platforms. See build log here: https://dev.azure.com/bmeng/GitHub/_build/results?buildId=152&view=results Could you please post a fix patch that could be squashed into this patch? This patch is already applied in u-boot-x86/master. Regards, Bin
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
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 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(-)