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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 12 October 2018 at 17:06, Sami Mujawar <Sami.Mujawar@arm.com> wrote:
> 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.
>

Yes. Just keep PcdSerialPciDeviceInfo at its default of 0xff, and the
PCI dependencies in that driver are essentially circumvented, so you
can use any PciLib implementation as a dummy (or create a new one that
ASSERT()s in all functions if you prefer)

> 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)'
>>
>>