[SeaBIOS] Re: [PATCH 4/4] serialio: Support for pci serial ports

[Paul Menzel]

Dear gaobin,

Thank you for your changes. Some nit picks:

Please configure git with `git config --global user.name "…"` to use
your real name. You can also use `git send-email` to send the changes.

On 2019-11-26 04:27, Your Real Name wrote:
> From f65c435b8e3caf7249a3fb25150e7055898ccc12 Mon Sep 17 00:00:00 2001
> From: gaobin 
> Date: Fri, 18 Oct 2019 23:00:21 -0700
> Subject: [PATCH 4/4] serialio: Support for pci serial ports

Make it a statement.
> serialio: Support PCI serial ports
> serialio: Add support for PCI serial ports
> Some Intel PCHs integrate pci uarts which are used for serial
> port debugging. For compatibility purpose, BIOS implementation
> may assign 0x3f8 to the pci uart's io bar at PEI stage, later
> during DXE stage the pci uart's bar is re-assigned to a different
> address. As a result, we can't use the hard coded IO port 0x3f8
> in SeaBIOS for debugging. Instead, we need read the port base

need *to* read
> address from the pci uart's BAR, either an IO BAR, or a 32bit
> memory BAR. This patch adds support for pci serial port debugging.

Please add a command how to test this with QEMU for example.
> Signed-off-by: gaobin
>
> src/Kconfig       | 34 +++++++++++++++++++++++++++++++++-
>  src/hw/serialio.c | 29 +++++++++++++++++++++++++++--
>  2 files changed, 60 insertions(+), 3 deletions(-)
>
> diff --git a/src/Kconfig b/src/Kconfig
> index 6606ce4..6d9ce3b 100644
> --- a/src/Kconfig
> +++ b/src/Kconfig
> @@ -550,8 +550,40 @@ menu "Debugging"
>          default 0x3f8
>          help
>              Base port for serial - generally 0x3f8, 0x2f8, 0x3e8, or 0x2e8.
> config DEBUG_SERIAL_MMIO
> config DEBUG_SERIAL_PCI_IO
>     depends on DEBUG_LEVEL != 0 && !DEBUG_SERIAL
>    bool "Serial port debugging via PCI IO"
>    default n
>    help
>        Send debugging information to PCI serial port.
>        The base address of the uart is in the PCI device's IO bar.

Please spell it UART.
> config DEBUG_SERIAL_PCI_MEM32
>    depends on DEBUG_LEVEL != 0 && !DEBUG_SERIAL && !DEBUG_SERIAL_PCI_IO
>    bool "Serial port debugging via PCI MEM32"
>    default n
>    help
>        Send debugging information to PCI serial port.
>        The base address of the uart is in the PCI device's MEM32 bar.
> config DEBUG_SERIAL_PCI_BDF
>    depends on DEBUG_SERIAL_PCI_IO || DEBUG_SERIAL_PCI_MEM32
>    hex "Serial port PCI bus/device/function"
>    range 0 ffff
>    help
>        16bit hex of B:D:F of the PCI serial port.
>        Bit 15:8 - bus, bit 7:3 - device, bit 2:0 - function
>        E.g. for a PCI device: bus 0x0, dev 0x1a, func 0x1 you
>        should input 00d1.
> config DEBUG_SERIAL_PCI_BAR
>    depends on DEBUG_SERIAL_PCI_IO || DEBUG_SERIAL_PCI_MEM32
>    hex "Serial port PCI BAR index (0 - 5)"
>    range 0 5
>    default 0
>    help
>        The index of the BAR where the base address would be read
>        from for the PCI serial port. Only IO BAR and 32bit memory
>        BAR are supported.
> config DEBUG_SERIAL_MMIO
>    depends on DEBUG_LEVEL != 0 && !DEBUG_SERIAL && !DEBUG_SERIAL_PCI_IO && !DEBUG_SERIAL_PCI_MEM32
>    bool "Serial port debugging via memory mapped IO"
>    default n
>    help
>
> diff --git a/src/hw/serialio.c b/src/hw/serialio.c
> index 3163344..9a8565c 100644
> --- a/src/hw/serialio.c
> +++ b/src/hw/serialio.c
> @@ -9,6 +9,7 @@
>  #include "output.h" // dprintf
>  #include "serialio.h" // serial_debug_preinit
>  #include "x86.h" // outb
> +#include "pci.h" //pci_config_readw
>
> /****************************************************************
> @@ -23,6 +24,15 @@ serial_debug_write(u8 offset, u8 val)
>  {
>      if (CONFIG_DEBUG_SERIAL) {
>          outb(val, CONFIG_DEBUG_SERIAL_PORT + offset);
> } else if (CONFIG_DEBUG_SERIAL_PCI_IO) {
>    u16 base = pci_config_readw(CONFIG_DEBUG_SERIAL_PCI_BDF,
>        0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x3);
>        outb(val, base + offset);
> } else if (CONFIG_DEBUG_SERIAL_PCI_MEM32) {
>    ASSERT32FLAT();
> u32 base = pci_config_readl(CONFIG_DEBUG_SERIAL_PCI_BDF,

Please use spaces for indentation.
>            0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x7);
>    writeb((void*)base + 4*offset, val);
> } else if (CONFIG_DEBUG_SERIAL_MMIO) {
>        ASSERT32FLAT();
>        writeb((void*)CONFIG_DEBUG_SERIAL_MEM_ADDRESS + 4*offset, val);
>
> @@ -35,6 +45,17 @@ serial_debug_read(u8 offset)
>  {
>      if (CONFIG_DEBUG_SERIAL)
>          return inb(CONFIG_DEBUG_SERIAL_PORT + offset);
> if (CONFIG_DEBUG_SERIAL_PCI_IO) {
>        u16 base = pci_config_readw(CONFIG_DEBUG_SERIAL_PCI_BDF,
>     0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x3);
>        return inb(base + offset);

Please fix the indentation/alignment to follow the coding style.
Also below.
> }
> if (CONFIG_DEBUG_SERIAL_PCI_MEM32) {
>    ASSERT32FLAT();
> u32 base = pci_config_readl(CONFIG_DEBUG_SERIAL_PCI_BDF,
>            0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x7);
>    return readb((void*)base + 4*offset);
> }
> if (CONFIG_DEBUG_SERIAL_MMIO) {
>     ASSERT32FLAT();
>     return readb((void*)CONFIG_DEBUG_SERIAL_MEM_ADDRESS + 4*offset);
>
> @@ -65,7 +86,9 @@ serial_debug_preinit(void)
>  static void
>  serial_debug(char c)
>  {
> if (!CONFIG_DEBUG_SERIAL && (!CONFIG_DEBUG_SERIAL_MMIO || MODESEGMENT))
> if (!CONFIG_DEBUG_SERIAL && !CONFIG_DEBUG_SERIAL_PCI_IO &&
>    ((!CONFIG_DEBUG_SERIAL_MMIO && !CONFIG_DEBUG_SERIAL_PCI_MEM32) ||
>    MODESEGMENT))
>    return;
> int timeout = DEBUG_TIMEOUT;
>    while ((serial_debug_read(SEROFF_LSR) & 0x20) != 0x20)
>
> @@ -87,7 +110,9 @@ serial_debug_putc(char c)
>  void
>  serial_debug_flush(void)
>  {
> if (!CONFIG_DEBUG_SERIAL && (!CONFIG_DEBUG_SERIAL_MMIO || MODESEGMENT))
> if (!CONFIG_DEBUG_SERIAL && !CONFIG_DEBUG_SERIAL_PCI_IO &&
>    ((!CONFIG_DEBUG_SERIAL_MMIO && !CONFIG_DEBUG_SERIAL_PCI_MEM32) ||
>    MODESEGMENT))
>    return;
> int timeout = DEBUG_TIMEOUT;
>    while ((serial_debug_read(SEROFF_LSR) & 0x60) != 0x60)

Kind regards,

Paul