* [Patch 1/1] MdeModulePkg/PciSioSerialDxe: Flush Tx before config change
@ 2020-12-11 0:32 Michael D Kinney
2020-12-11 3:22 ` [edk2-devel] " Ni, Ray
2020-12-11 5:56 ` Wu, Hao A
0 siblings, 2 replies; 3+ messages in thread
From: Michael D Kinney @ 2020-12-11 0:32 UTC (permalink / raw)
To: devel; +Cc: Hao A Wu, Ray Ni
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [Patch 1/1] MdeModulePkg/PciSioSerialDxe: Flush Tx before config change
2020-12-11 0:32 [Patch 1/1] MdeModulePkg/PciSioSerialDxe: Flush Tx before config change Michael D Kinney
@ 2020-12-11 3:22 ` Ni, Ray
2020-12-11 5:56 ` Wu, Hao A
1 sibling, 0 replies; 3+ messages in thread
From: Ni, Ray @ 2020-12-11 3:22 UTC (permalink / raw)
To: devel@edk2.groups.io, Kinney, Michael D; +Cc: Wu, Hao A
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----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
>
>
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [Patch 1/1] MdeModulePkg/PciSioSerialDxe: Flush Tx before config change
2020-12-11 0:32 [Patch 1/1] MdeModulePkg/PciSioSerialDxe: Flush Tx before config change Michael D Kinney
2020-12-11 3:22 ` [edk2-devel] " Ni, Ray
@ 2020-12-11 5:56 ` Wu, Hao A
1 sibling, 0 replies; 3+ messages in thread
From: Wu, Hao A @ 2020-12-11 5:56 UTC (permalink / raw)
To: devel@edk2.groups.io, Kinney, Michael D; +Cc: Ni, Ray
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
>
>
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-12-11 5:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-11 0:32 [Patch 1/1] MdeModulePkg/PciSioSerialDxe: Flush Tx before config change Michael D Kinney
2020-12-11 3:22 ` [edk2-devel] " Ni, Ray
2020-12-11 5:56 ` Wu, Hao A
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox