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

[Bin Meng]

Hi Simon,

On Wed, Dec 11, 2019 at 3:35 AM Park, Aiden <aiden.park at intel.com> wrote:
>
> Hi Simon,
>
> > -----Original Message-----
> > From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Simon Glass
> > Sent: Monday, December 9, 2019 8:59 AM
> > To: U-Boot Mailing List <u-boot at lists.denx.de>
> > Cc: Stefan Roese <sr at denx.de>; Angelo Dureghello <angelo at 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 at 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);
> > +     }
> > +}
> 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