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

Hi Bin,

On Mon, 16 Dec 2019 at 00:03, Bin Meng bmeng.cn@gmail.com wrote:

Hi Simon,

On Wed, Dec 11, 2019 at 3:35 AM Park, Aiden aiden.park@intel.com wrote:

...

Hi Simon,

...

-----Original Message----- From: U-Boot u-boot-bounces@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 sr@denx.de; Angelo Dureghello angelo@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@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