Hi Bin, On Mon, 16 Dec 2019 at 00:03, Bin Meng <bmeng...@gmail.com> wrote: > > Hi Simon, > > On Wed, Dec 11, 2019 at 3:35 AM Park, Aiden <aiden.p...@intel.com> wrote: > > > > Hi Simon, > > > > > -----Original Message----- > > > From: U-Boot <u-boot-boun...@lists.denx.de> On Behalf Of Simon Glass > > > Sent: Monday, December 9, 2019 8:59 AM > > > To: U-Boot Mailing List <u-boot@lists.denx.de> > > > Cc: Stefan Roese <s...@denx.de>; Angelo Dureghello <ang...@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 <s...@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