From mboxrd@z Thu Jan 1 00:00:00 1970 From: Park, Aiden Date: Mon, 9 Dec 2019 23:50:05 +0000 Subject: [PATCH 1/4] serial: n16550: Support run-time configuration In-Reply-To: References: <20191205230428.23497-1-sjg@chromium.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > Sent: Sunday, December 8, 2019 3:31 AM > To: Simon Glass ; Park, Aiden > Cc: U-Boot Mailing List > 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 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 > > --- > > > > 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