[...]


The above analysis is very clear, thanks. I am a little concern about if
the code changes below follow the comments in the code.

In Terminal.c:
     //
     // Set the timeout value of serial buffer for
     // keystroke response performance issue
     //
In TerminalConIn.c:
   //
   //  if current timeout value for serial device is not identical with
   //  the value saved in TERMINAL_DEV structure, then recalculate the
   //  timeout value again and set serial attribute according to this
value.
   //

And I also checked the
IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c although ARM
platforms do not use it, the SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH
value is 1. if SetAttributes() is called with ReceiveFifoDepth = 0, then
SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH is selected as the description of
the functions is "A ReceiveFifoDepth value of 0 will use the device's
default FIFO depth", so what's the device's default FIFO depth?

I have a thought that updates SerialDxe to call SerialSetAttributes(with
0, 0, 0, DefaultParity, 0 and DefaultStopBits) in SerialDxeInitialize()
and SerialReset() to get the real default values of the device, then the
driver can also eliminate the use of PcdUartDefault####.

Try to explain the thought by below patch (not tested yet).

3cc4f3e688471946048de7ea67e0a73631844fad
 MdeModulePkg/Universal/SerialDxe/SerialDxe.inf |  7 ---
MdeModulePkg/Universal/SerialDxe/SerialIo.c | 69 +++++++++++++-------------
 2 files changed, 35 insertions(+), 41 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf b/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
index 164060b..9e35c03 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
+++ b/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
@@ -33,19 +33,12 @@
   UefiDriverEntryPoint
   UefiBootServicesTableLib
   DebugLib
-  PcdLib
   SerialPortLib

 [Protocols]
   gEfiSerialIoProtocolGuid      ## PRODUCES
   gEfiDevicePathProtocolGuid    ## PRODUCES

-[Pcd]
-  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate ## CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits ## CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity   ## CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits ## CONSUMES
-
 [Depex]
   TRUE

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index de928d1..3985598 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -18,7 +18,6 @@
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/SerialPortLib.h>
 #include <Library/DebugLib.h>
-#include <Library/PcdLib.h>

 #include <Protocol/SerialIo.h>
 #include <Protocol/DevicePath.h>
@@ -180,15 +179,7 @@ SERIAL_DEVICE_PATH mSerialDevicePath = {
 //
 // Template used to initialize the Serial IO protocols.
 //
-EFI_SERIAL_IO_MODE mSerialIoMode = {
-  0, // ControlMask
-  0, // Timeout
-  0, // BaudRate
-  1, // ReceiveFifoDepth
-  0, // DataBits
-  0, // Parity
-  0  // StopBits
-};
+EFI_SERIAL_IO_MODE mSerialIoMode;

 EFI_SERIAL_IO_PROTOCOL mSerialIoTemplate = {
   SERIAL_IO_INTERFACE_REVISION,
@@ -201,6 +192,34 @@ EFI_SERIAL_IO_PROTOCOL mSerialIoTemplate = {
   &mSerialIoMode
 };

+EFI_STATUS
+SerialDeviceInitialize (
+  IN EFI_SERIAL_IO_MODE *SerialIoMode
+  )
+{
+  EFI_STATUS    Status;
+
+  Status = SerialPortInitialize ();
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  //
+  // Get the real default value of the device.
+  //
+  ZeroMem (&SerialIoMode, sizeof (SerialIoMode));
+  Status = SerialPortSetAttributes (
+             &SerialIoMode->BaudRate,
+             &SerialIoMode->ReceiveFifoDepth,
+             &SerialIoMode->Timeout,
+             (EFI_PARITY_TYPE *) &SerialIoMode->Parity,
+             &SerialIoMode->DataBits,
+             (EFI_STOP_BITS_TYPE *) &SerialIoMode->StopBits);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+}
+
 /**
   Reset the serial device.

@@ -219,28 +238,14 @@ SerialReset (
   EFI_STATUS    Status;
   EFI_TPL       Tpl;

-  Status = SerialPortInitialize ();
+  Status = SerialDeviceInitialize (&This->Mode);
   if (EFI_ERROR (Status)) {
     return Status;
   }

-  //
-  // Set the Serial I/O mode and update the device path
-  //
-
   Tpl = gBS->RaiseTPL (TPL_NOTIFY);

   //
-  // Set the Serial I/O mode
-  //
-  This->Mode->ReceiveFifoDepth  = 1;
-  This->Mode->Timeout           = 0;
-  This->Mode->BaudRate          = PcdGet64 (PcdUartDefaultBaudRate);
- This->Mode->DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
-  This->Mode->Parity            = (UINT32) PcdGet8 (PcdUartDefaultParity);
- This->Mode->StopBits = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
-
-  //
   // Check if the device path has actually changed
   //
   if (mSerialDevicePath.Uart.BaudRate == This->Mode->BaudRate &&
@@ -496,19 +501,15 @@ SerialDxeInitialize (
 {
   EFI_STATUS            Status;

-  Status = SerialPortInitialize ();
+  Status = SerialDeviceInitialize (&mSerialIoMode);
   if (EFI_ERROR (Status)) {
     return Status;
   }

-  mSerialIoMode.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
-  mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
-  mSerialIoMode.Parity   = (UINT32) PcdGet8 (PcdUartDefaultParity);
-  mSerialIoMode.StopBits = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
-  mSerialDevicePath.Uart.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
-  mSerialDevicePath.Uart.DataBits = PcdGet8 (PcdUartDefaultDataBits);
-  mSerialDevicePath.Uart.Parity   = PcdGet8 (PcdUartDefaultParity);
-  mSerialDevicePath.Uart.StopBits = PcdGet8 (PcdUartDefaultStopBits);
+  mSerialDevicePath.Uart.BaudRate = mSerialIoMode.BaudRate;
+  mSerialDevicePath.Uart.DataBits = (UINT8) mSerialIoMode.DataBits;
+  mSerialDevicePath.Uart.Parity   = (UINT8) mSerialIoMode.Parity;
+  mSerialDevicePath.Uart.StopBits = (UINT8) mSerialIoMode.StopBits;

   //
   // Make a new handle with Serial IO protocol and its device path on it.



What's your opinion?

Thanks,
Star


[...]
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to