Re: [PATCH v1 1/6] PcAtChipsetPkg: Add MMIO Support to SerialIo Lib

[Sami Mujawar <Sami.Mujawar@arm.com>]

Hi Ard,

I believe you are referring to MdeModulePkg\Library\BaseSerialPortLib16550\BaseSerialPortLib16550.inf
I have tried using this Serial Port Library. However, this has a dependency on PciLib which is difficult to resolve as we use the Serial port early in the boot.
Please do let me know if you know a workaround to solve this issue.

Regards,

Sami Mujawar

-----Original Message-----
From: Ard Biesheuvel <ard.biesheuvel@linaro.org> 
Sent: 12 October 2018 03:50 PM
To: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>; Ruiyu Ni <ruiyu.ni@intel.com>; Evan Lloyd <Evan.Lloyd@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; nd <nd@arm.com>
Subject: Re: [PATCH v1 1/6] PcAtChipsetPkg: Add MMIO Support to SerialIo Lib

On 12 October 2018 at 16:40, Sami Mujawar <sami.mujawar@arm.com> wrote:
> Some virtual machine managers like kvmtool emulate the PC AT Serial 
> Port UART in the MMIO space so that architectures that do not support 
> I/O Mapped I/O can use the UART.
>
> This patch adds MMIO support to the PC AT SerialPortLib.
>
> PcdSerialUseMmio is used to select I/O or MMIO support.
>   If PcdSerialUseMmio is
>     TRUE  - The value is read/written from MMIO space.
>     FALSE - The value is read/written from I/O space.
>             The Default is I/O space.
>

IIUC kvmtool exposes a 8250, and our 8250 driver already implements support for MMIO mapped registers. Can't you use that instead?

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
> The changes can be seen at 
> https://github.com/samimujawar/edk2/commit/84f908387d2031def5d374759e7
> ad4cf90786c91
>
> Notes:
>     v1:
>     - Add support to read/write from UART registers using MMIO access 
> [SAMI]
>
>  PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf |  4 +  
> PcAtChipsetPkg/Library/SerialIoLib/SerialPortLib.c | 98 
> ++++++++++++++++----
>  2 files changed, 84 insertions(+), 18 deletions(-)
>
> diff --git a/PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf 
> b/PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
> index 
> 959d6e27c9812d201d44d9249070ab7758cbfe00..d0689793040fd930701b02dae51f
> f59ea16a10c4 100644
> --- a/PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
> +++ b/PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
> @@ -2,6 +2,7 @@
>  #   Library instance for SerialIo library class
>  #
>  #  Copyright (c) 2006 - 2014, Intel Corporation. All rights 
> reserved.<BR>
> +#  Copyright (c) 2018, ARM Limited. All rights reserved.<BR>
>  #  This program and the accompanying materials  #  are licensed and 
> made available under the terms and conditions of the BSD License  #  
> which accompanies this distribution.  The full text of the license may 
> be found at @@ -23,6 +24,7 @@ [Defines]
>
>  [Packages]
>    MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
>
>  [LibraryClasses]
>    BaseLib
> @@ -31,3 +33,5 @@ [LibraryClasses]
>  [Sources]
>    SerialPortLib.c
>
> +[FixedPcd]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio                 ## CONSUMES
> diff --git a/PcAtChipsetPkg/Library/SerialIoLib/SerialPortLib.c 
> b/PcAtChipsetPkg/Library/SerialIoLib/SerialPortLib.c
> index 
> d1a1c6a03facad09e781b5605e22a24e5f51c618..898e5f957aae998624189c6b5389
> 12da0439dfe8 100644
> --- a/PcAtChipsetPkg/Library/SerialIoLib/SerialPortLib.c
> +++ b/PcAtChipsetPkg/Library/SerialIoLib/SerialPortLib.c
> @@ -2,6 +2,8 @@
>    UART Serial Port library functions
>
>    Copyright (c) 2006 - 2018, Intel Corporation. All rights 
> reserved.<BR>
> +  Copyright (c) 2018, ARM Limited. All rights reserved.<BR>
> +
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD License
>    which accompanies this distribution.  The full text of the license may be found at
> @@ -57,6 +59,66 @@ UINT8   gParity   = 0;
>  UINT8   gBreakSet = 0;
>
>  /**
> +  Reads an 8-bit value from the serial port.
> +
> +  This function checks PcdSerialUseMmio to determine if I/O  mapped 
> + IO or Memory Mapped IO operations must be performed on  the serial 
> + port. It then uses the appropriate interface to  read the Value from 
> + the serial port.
> +
> +  If PcdSerialUseMmio is TRUE, then the value is read from MMIO space.
> +  If PcdSerialUseMmio is FALSE, then the value is read from I/O space.
> +
> +  @param  Address   The UART register to read from.
> +
> +  @return The value read from the serial port.
> +
> +**/
> +STATIC
> +UINT8
> +SerialPortRead8 (
> +  IN  UINTN   Address
> +)
> +{
> +  if (FixedPcdGetBool (PcdSerialUseMmio)) {
> +    return MmioRead8 (Address);
> +  }
> +
> +  return IoRead8 (Address);
> +}
> +
> +/**
> +  Writes an 8-bit value to the serial port.
> +
> +  This function checks PcdSerialUseMmio to determine if I/O  mapped 
> + IO or Memory Mapped IO operations must be performed on  the serial 
> + port. It then uses the appropriate interface to  write the Value to 
> + the serial port.
> +
> +  If PcdSerialUseMmio is TRUE, then the value is written to MMIO space.
> +  If PcdSerialUseMmio is FALSE, then the value is written to I/O space.
> +
> +  @param  Address   The UART register to write.
> +  @param  Value     The value to write to the I/O port.
> +
> +  @return The value written to the serial port.
> +
> +**/
> +STATIC
> +UINT8
> +SerialPortWrite8 (
> +  IN  UINTN   Address,
> +  IN  UINT8   Value
> +)
> +{
> +  if (FixedPcdGetBool (PcdSerialUseMmio)) {
> +    return MmioWrite8 (Address, Value);
> +  }
> +
> +  return IoWrite8 (Address, Value);
> +}
> +
> +/**
>    Initialize the serial device hardware.
>
>    If no initialization is required, then return RETURN_SUCCESS.
> @@ -91,19 +153,19 @@ SerialPortInitialize (
>    // Set communications format
>    //
>    OutputData = (UINT8) ((DLAB << 7) | (gBreakSet << 6) | (gParity << 
> 3) | (gStop << 2) | Data);
> -  IoWrite8 (gUartBase + LCR_OFFSET, OutputData);
> +  SerialPortWrite8 (gUartBase + LCR_OFFSET, OutputData);
>
>    //
>    // Configure baud rate
>    //
> -  IoWrite8 (gUartBase + BAUD_HIGH_OFFSET, (UINT8) (Divisor >> 8));
> -  IoWrite8 (gUartBase + BAUD_LOW_OFFSET, (UINT8) (Divisor & 0xff));
> +  SerialPortWrite8 (gUartBase + BAUD_HIGH_OFFSET, (UINT8) (Divisor >> 
> + 8));
> +  SerialPortWrite8 (gUartBase + BAUD_LOW_OFFSET, (UINT8) (Divisor & 
> + 0xff));
>
>    //
>    // Switch back to bank 0
>    //
>    OutputData = (UINT8) ( (gBreakSet << 6) | (gParity << 3) | (gStop 
> << 2) | Data);
> -  IoWrite8 (gUartBase + LCR_OFFSET, OutputData);
> +  SerialPortWrite8 (gUartBase + LCR_OFFSET, OutputData);
>
>    return RETURN_SUCCESS;
>  }
> @@ -148,9 +210,9 @@ SerialPortWrite (
>      // Wait for the serail port to be ready.
>      //
>      do {
> -      Data = IoRead8 ((UINT16) gUartBase + LSR_OFFSET);
> +      Data = SerialPortRead8 ((UINT16) gUartBase + LSR_OFFSET);
>      } while ((Data & LSR_TXRDY) == 0);
> -    IoWrite8 ((UINT16) gUartBase, *Buffer++);
> +    SerialPortWrite8 ((UINT16) gUartBase, *Buffer++);
>    }
>
>    return Result;
> @@ -189,10 +251,10 @@ SerialPortRead (
>      // Wait for the serail port to be ready.
>      //
>      do {
> -      Data = IoRead8 ((UINT16) gUartBase + LSR_OFFSET);
> +      Data = SerialPortRead8 ((UINT16) gUartBase + LSR_OFFSET);
>      } while ((Data & LSR_RXDA) == 0);
>
> -    *Buffer++ = IoRead8 ((UINT16) gUartBase);
> +    *Buffer++ = SerialPortRead8 ((UINT16) gUartBase);
>    }
>
>    return Result;
> @@ -220,7 +282,7 @@ SerialPortPoll (
>    //
>    // Read the serial port status.
>    //
> -  Data = IoRead8 ((UINT16) gUartBase + LSR_OFFSET);
> +  Data = SerialPortRead8 ((UINT16) gUartBase + LSR_OFFSET);
>
>    return (BOOLEAN) ((Data & LSR_RXDA) != 0);  } @@ -253,7 +315,7 @@ 
> SerialPortSetControl (
>    //
>    // Read the Modem Control Register.
>    //
> -  Mcr = IoRead8 ((UINT16) gUartBase + MCR_OFFSET);
> +  Mcr = SerialPortRead8 ((UINT16) gUartBase + MCR_OFFSET);
>    Mcr &= (~(MCR_DTRC | MCR_RTS));
>
>    if ((Control & EFI_SERIAL_DATA_TERMINAL_READY) == 
> EFI_SERIAL_DATA_TERMINAL_READY) { @@ -267,7 +329,7 @@ SerialPortSetControl (
>    //
>    // Write the Modem Control Register.
>    //
> -  IoWrite8 ((UINT16) gUartBase + MCR_OFFSET, Mcr);
> +  SerialPortWrite8 ((UINT16) gUartBase + MCR_OFFSET, Mcr);
>
>    return RETURN_SUCCESS;
>  }
> @@ -297,7 +359,7 @@ SerialPortGetControl (
>    //
>    // Read the Modem Status Register.
>    //
> -  Msr = IoRead8 ((UINT16) gUartBase + MSR_OFFSET);
> +  Msr = SerialPortRead8 ((UINT16) gUartBase + MSR_OFFSET);
>
>    if ((Msr & MSR_CTS) == MSR_CTS) {
>      *Control |= EFI_SERIAL_CLEAR_TO_SEND; @@ -318,7 +380,7 @@ 
> SerialPortGetControl (
>    //
>    // Read the Modem Control Register.
>    //
> -  Mcr = IoRead8 ((UINT16) gUartBase + MCR_OFFSET);
> +  Mcr = SerialPortRead8 ((UINT16) gUartBase + MCR_OFFSET);
>
>    if ((Mcr & MCR_DTRC) == MCR_DTRC) {
>      *Control |= EFI_SERIAL_DATA_TERMINAL_READY; @@ -331,7 +393,7 @@ 
> SerialPortGetControl (
>    //
>    // Read the Line Status Register.
>    //
> -  Lsr = IoRead8 ((UINT16) gUartBase + LSR_OFFSET);
> +  Lsr = SerialPortRead8 ((UINT16) gUartBase + LSR_OFFSET);
>
>    if ((Lsr & LSR_TXRDY) == LSR_TXRDY) {
>      *Control |= EFI_SERIAL_OUTPUT_BUFFER_EMPTY; @@ -470,19 +532,19 @@ 
> SerialPortSetAttributes (
>    // Set communications format
>    //
>    OutputData = (UINT8) ((DLAB << 7) | (gBreakSet << 6) | (LcrParity 
> << 3) | (LcrStop << 2) | LcrData);
> -  IoWrite8 (gUartBase + LCR_OFFSET, OutputData);
> +  SerialPortWrite8 (gUartBase + LCR_OFFSET, OutputData);
>
>    //
>    // Configure baud rate
>    //
> -  IoWrite8 (gUartBase + BAUD_HIGH_OFFSET, (UINT8) (Divisor >> 8));
> -  IoWrite8 (gUartBase + BAUD_LOW_OFFSET, (UINT8) (Divisor & 0xff));
> +  SerialPortWrite8 (gUartBase + BAUD_HIGH_OFFSET, (UINT8) (Divisor >> 
> + 8));
> +  SerialPortWrite8 (gUartBase + BAUD_LOW_OFFSET, (UINT8) (Divisor & 
> + 0xff));
>
>    //
>    // Switch back to bank 0
>    //
>    OutputData = (UINT8) ((gBreakSet << 6) | (LcrParity << 3) | 
> (LcrStop << 2) | LcrData);
> -  IoWrite8 (gUartBase + LCR_OFFSET, OutputData);
> +  SerialPortWrite8 (gUartBase + LCR_OFFSET, OutputData);
>
>    return RETURN_SUCCESS;
>  }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>