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

[Simon Glass]

Hi Bin,

On Mon, 16 Dec 2019 at 00:03, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> 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?

Yes, but actually I have found that we need one more case, so it needs
a bit more effort. Will post an update series by Monday.

Regards,
SImon