[PATCH 1/4] serial: n16550: Support run-time configuration
Park, Aiden
aiden.park at intel.com
Tue Dec 10 00:50:05 CET 2019
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 at gmail.com>
> Sent: Sunday, December 8, 2019 3:31 AM
> To: Simon Glass <sjg at chromium.org>; Park, Aiden <aiden.park at intel.com>
> Cc: U-Boot Mailing List <u-boot at 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 at 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 at 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
More information about the U-Boot
mailing list