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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Sami,

The BaseSerialPortLib16550 supports I/O port, MMIO, and PCI based UARTs.

PCDs are used to select the modes.

I would like to see the BaseSerialPortLib16550 used instead of
the serial port lib in PcAtChipsetPkg.

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> On Behalf Of Sami Mujawar
> Sent: Friday, October 12, 2018 8:06 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; nd <nd@arm.com>;
> Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>;
> edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v1 1/6] PcAtChipsetPkg: Add
> MMIO Support to SerialIo Lib
> 
> Hi Ard,
> 
> I believe you are referring to
> MdeModulePkg\Library\BaseSerialPortLib16550\BaseSerialPor
> tLib16550.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/84f908387d2031
> def5d374759e7
> > 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..d0689793040fd93
> 0701b02dae51f
> > 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..898e5f957aae998
> 624189c6b5389
> > 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)'
> >
> >
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel