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

[Bin Meng <bmeng.cn@gmail.com>]

+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