Re: [edk2-devel] [Patch 1/1] MdeModulePkg/PciSioSerialDxe: Flush Tx before config change

["Wu, Hao A" <hao.a.wu@intel.com>]

Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
> D Kinney
> Sent: Friday, December 11, 2020 8:33 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [edk2-devel] [Patch 1/1] MdeModulePkg/PciSioSerialDxe: Flush Tx
> before config change
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=3114
> 
> Add logic to flush all UART transmit buffers if there is a config change from
> Reset(), SetAttributes() or SetControl().
> Use a timeout in the flush operation, so the system can continue to boot if
> the transmit buffers can not be flushed for any reason.
> 
> This change prevents lost characters on serial debug logs and serial consoles
> when a config change is made.  It also prevents a UART from getting into a
> bad state or reporting error status due to characters being transmitted at the
> same time registers are updated with new communications settings.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  .../Bus/Pci/PciSioSerialDxe/SerialIo.c        | 91 ++++++++++++++++++-
>  1 file changed, 90 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c
> b/MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c
> index 8377ffa13c7a..56c5faf5db1f 100644
> --- a/MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c
> @@ -1,7 +1,7 @@
>  /** @file
>    SerialIo implementation for PCI or SIO UARTs.
> 
> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -436,6 +436,68 @@ SerialReceiveTransmit (
>    return EFI_SUCCESS;
>  }
> 
> +/**
> +  Flush the serial hardware transmit FIFO, holding register, and shift register.
> +
> +  @param SerialDevice  The device to flush.
> +
> +  @retval  EFI_SUCCESS  The transmit FIFO is completely flushed.
> +  @retval  EFI_TIMEOUT  A timeout occured waiting for the transmit FIFO to
> flush.
> +**/
> +EFI_STATUS
> +SerialFlushTransmitFifo (
> +  SERIAL_DEV  *SerialDevice
> +  )
> +{
> +  SERIAL_PORT_LSR  Lsr;
> +  UINTN            Timeout;
> +  UINTN            Elapsed;
> +
> +  //
> +  // If this is the first time the UART is being configured, then the
> + current  // UART settings are not known, so compute a timeout to wait
> + for the Tx FIFO  // assuming the worst case current settings.
> +  //
> +  // Timeout = (Max Bits per Char) * (Max Pending Chars) / (Slowest Baud
> Rate)
> +  //   Max Bits per Char = Start bit + 8 data bits + parity + 2 stop bits = 12
> +  //   Max Pending Chars = Largest Tx FIFO + hold + shift = 64 + 1 + 1 = 66
> +  //   Slowest Reasonable Baud Rate = 300 baud
> +  // Timeout = 12 * 66 / 300 = 2.64 seconds = 2,640,000 uS  //  Timeout
> + = 2640000;
> +
> +  //
> +  // Use the largest of the computed timeout, the default timeout, and
> + the  // currently set timeout.
> +  //
> +  Timeout = MAX (Timeout, SERIAL_PORT_DEFAULT_TIMEOUT);  Timeout =
> MAX
> + (Timeout, SerialDevice->SerialMode.Timeout);
> +
> +  //
> +  // Wait for the shortest time possible for the serial port to be
> + ready making  // sure the transmit FIFO, holding register, and shift
> + register are all  // empty.  The actual wait time is expected to be
> + very small because the  // number characters currently in the FIFO
> + should be small when a  // configuration change is requested.
> +  //
> +  // NOTE: Do not use any DEBUG() or REPORT_STATUS_CODE() or any other
> + calls  // in the rest of this function that may send additional
> + characters to this  // UART device invalidating the flush operation.
> +  //
> +  Elapsed = 0;
> +  Lsr.Data = READ_LSR (SerialDevice);
> +  while (Lsr.Bits.Temt == 0 || Lsr.Bits.Thre == 0) {
> +    if (Elapsed >= Timeout) {
> +      return EFI_TIMEOUT;
> +    }
> +    gBS->Stall (TIMEOUT_STALL_INTERVAL);
> +    Elapsed += TIMEOUT_STALL_INTERVAL;
> +    Lsr.Data = READ_LSR (SerialDevice);  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>  //
>  // Interface Functions
>  //
> @@ -476,6 +538,15 @@ SerialReset (
> 
>    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> 
> +  //
> +  // Wait for all data to be transmitted before changing the UART
> configuration.
> +  //
> +  // NOTE: Do not use any DEBUG() or REPORT_STATUS_CODE() or any other
> + calls  // that may send additional characters to this UART device
> + until the UART  // configuration change is complete.
> +  //
> +  SerialFlushTransmitFifo (SerialDevice);
> +
>    //
>    // Make sure DLAB is 0.
>    //
> @@ -654,6 +725,15 @@ SerialSetAttributes (
> 
>    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> 
> +  //
> +  // Wait for all data to be transmitted before changing the UART
> configuration.
> +  //
> +  // NOTE: Do not use any DEBUG() or REPORT_STATUS_CODE() or any other
> + calls  // that may send additional characters to this UART device
> + until the UART  // configuration change is complete.
> +  //
> +  SerialFlushTransmitFifo (SerialDevice);
> +
>    //
>    // Put serial port on Divisor Latch Mode
>    //
> @@ -826,6 +906,15 @@ SerialSetControl (
> 
>    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> 
> +  //
> +  // Wait for all data to be transmitted before changing the UART
> configuration.
> +  //
> +  // NOTE: Do not use any DEBUG() or REPORT_STATUS_CODE() or any other
> + calls  // that may send additional characters to this UART device
> + until the UART  // configuration change is complete.
> +  //
> +  SerialFlushTransmitFifo (SerialDevice);
> +
>    Mcr.Data = READ_MCR (SerialDevice);
>    Mcr.Bits.DtrC = 0;
>    Mcr.Bits.Rts = 0;
> --
> 2.29.2.windows.2
> 
> 
> 
> 
>