Re: [edk2] [PATCH V3 11/12] ArmVirtPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Zeng, Star]

More comments below.

On 2015/11/24 9:01, Zeng, Star wrote:
Laszlo,
Explain more below.

On 2015/11/23 22:01, Laszlo Ersek wrote:
Star,

On 11/23/15 02:44, Star Zeng wrote:
Beyond just changing the directly related lines in the FDF and DSC
files,
we have to adapt the EarlyFdtPL011SerialPortLib and
FdtPL011SerialPortLib
instances as well, in the same patch. This is because the EmbeddedPkg
driver expects the SerialPortSetAttributes(),
SerialPortSetControl() and SerialPortGetControl() functions from
SerialPortExtLib, while the MdeModulePkg driver expects them from
SerialPortLib itself.

We cannot implement these functions in ArmVirtPkg's SerialPortLib
instances *before* flipping the driver, because it would cause double
function definitions in the EmbeddedPkg driver. We also can't implement
the functions *after* flipping the driver, because it would cause
unresolved function references in the MdeModulePkg driver. Therefore
we have to implement the functions simultaneously with the driver
replacement.

(1) The commit message has been updated the way I asked, thank you for
that. (See also (3) below.)

However,


Cc: Michael D Kinney <michael.d.kin...@intel.com>
Cc: Liming Gao <liming....@intel.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng <star.z...@intel.com>
---
  ArmVirtPkg/ArmVirt.dsc.inc                         |  1 -
  ArmVirtPkg/ArmVirtQemu.dsc                         |  2 +-
  ArmVirtPkg/ArmVirtQemu.fdf                         |  2 +-
  ArmVirtPkg/ArmVirtXen.dsc                          |  3 +-
  ArmVirtPkg/ArmVirtXen.fdf                          |  3 +-
  .../EarlyFdtPL011SerialPortLib.c                   | 88
+++++++++++++++++++++-
  .../FdtPL011SerialPortLib/FdtPL011SerialPortLib.c  | 87
+++++++++++++++++++++
  7 files changed, 180 insertions(+), 6 deletions(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 8626919..b5821a8 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -101,7 +101,6 @@ [LibraryClasses.common]
    # ARM PL011 UART Driver
    PL011UartLib|ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf

SerialPortLib|ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf

-
SerialPortExtLib|EmbeddedPkg/Library/SerialPortExtLibNull/SerialPortExtLibNull.inf


    #
    # Uncomment (and comment out the next line) For RealView
Debugger. The Standard IO window
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 995be89..32c82c7 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -284,7 +284,7 @@ [Components.common]
    MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf

MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf
    MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
-  EmbeddedPkg/SerialDxe/SerialDxe.inf
+  MdeModulePkg/Universal/SerialDxe/SerialDxe.inf

    MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf

diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
index 89a4015..738e3db 100644
--- a/ArmVirtPkg/ArmVirtQemu.fdf
+++ b/ArmVirtPkg/ArmVirtQemu.fdf
@@ -134,7 +134,7 @@ [FV.FvMain]
    INF MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf
    INF
MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf
    INF MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
-  INF EmbeddedPkg/SerialDxe/SerialDxe.inf
+  INF MdeModulePkg/Universal/SerialDxe/SerialDxe.inf

    INF ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
    INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf
diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
index ac37cd2..32e0afc 100644
--- a/ArmVirtPkg/ArmVirtXen.dsc
+++ b/ArmVirtPkg/ArmVirtXen.dsc
@@ -1,6 +1,7 @@
  #
  #  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
  #  Copyright (c) 2014, Linaro Limited. All rights reserved.
+#  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
  #
  #  This program and the accompanying materials
  #  are licensed and made available under the terms and conditions
of the BSD License
@@ -198,7 +199,7 @@ [Components.common]

    MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf
    MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
-  EmbeddedPkg/SerialDxe/SerialDxe.inf
+  MdeModulePkg/Universal/SerialDxe/SerialDxe.inf

    MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf

diff --git a/ArmVirtPkg/ArmVirtXen.fdf b/ArmVirtPkg/ArmVirtXen.fdf
index 97cab4b..7290147 100644
--- a/ArmVirtPkg/ArmVirtXen.fdf
+++ b/ArmVirtPkg/ArmVirtXen.fdf
@@ -1,6 +1,7 @@
  #
  #  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
  #  Copyright (c) 2014, Linaro Limited. All rights reserved.
+#  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
  #
  #  This program and the accompanying materials
  #  are licensed and made available under the terms and conditions
of the BSD License
@@ -134,7 +135,7 @@ [FV.FvMain]
    #
    INF MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf
    INF MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
-  INF EmbeddedPkg/SerialDxe/SerialDxe.inf
+  INF MdeModulePkg/Universal/SerialDxe/SerialDxe.inf

    INF ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
    INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf
diff --git
a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c
b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c
index ba6d277..c8bfb29 100644
---
a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c
+++
b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c
@@ -4,6 +4,7 @@
    Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
    Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR>
    Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR>
+  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>

    This program and the accompanying materials
    are licensed and made available under the terms and conditions of
the BSD License
@@ -19,7 +20,6 @@

  #include <Library/PcdLib.h>
  #include <Library/SerialPortLib.h>
-#include <Library/SerialPortExtLib.h>
  #include <libfdt.h>

  #include <Drivers/PL011Uart.h>
@@ -183,3 +183,89 @@ SerialPortPoll (
  {
    return FALSE;
  }
+
+/**
+  Sets the control bits on a serial device.
+
+  @param[in] Control            Sets the bits of Control that are
settable.
+
+  @retval RETURN_SUCCESS        The new control bits were set on the
serial device.
+  @retval RETURN_UNSUPPORTED    The serial device does not support
this operation.
+  @retval RETURN_DEVICE_ERROR   The serial device is not functioning
correctly.
+
+**/
+RETURN_STATUS
+EFIAPI
+SerialPortSetControl (
+  IN UINT32 Control
+  )
+{
+  return RETURN_UNSUPPORTED;
+}
+
+/**
+  Retrieve the status of the control bits on a serial device.
+
+  @param[out] Control           A pointer to return the current
control signals from the serial device.
+
+  @retval RETURN_SUCCESS        The control bits were read from the
serial device.
+  @retval RETURN_UNSUPPORTED    The serial device does not support
this operation.
+  @retval RETURN_DEVICE_ERROR   The serial device is not functioning
correctly.
+
+**/
+RETURN_STATUS
+EFIAPI
+SerialPortGetControl (
+  OUT UINT32 *Control
+  )
+{
+  return RETURN_UNSUPPORTED;
+}
+
+/**
+  Sets the baud rate, receive FIFO depth, transmit/receice time out,
parity,
+  data bits, and stop bits on a serial device.
+
+  @param BaudRate           The requested baud rate. A BaudRate
value of 0 will use the
+                            device's default interface speed.
+                            On output, the value actually set.
+  @param ReveiveFifoDepth   The requested depth of the FIFO on the
receive side of the
+                            serial interface. A ReceiveFifoDepth
value of 0 will use
+                            the device's default FIFO depth.
+                            On output, the value actually set.
+  @param Timeout            The requested time out for a single
character in microseconds.
+                            This timeout applies to both the
transmit and receive side of the
+                            interface. A Timeout value of 0 will use
the device's default time
+                            out value.
+                            On output, the value actually set.
+  @param Parity             The type of parity to use on this serial
device. A Parity value of
+                            DefaultParity will use the device's
default parity value.
+                            On output, the value actually set.
+  @param DataBits           The number of data bits to use on the
serial device. A DataBits
+                            vaule of 0 will use the device's default
data bit setting.
+                            On output, the value actually set.
+  @param StopBits           The number of stop bits to use on this
serial device. A StopBits
+                            value of DefaultStopBits will use the
device's default number of
+                            stop bits.
+                            On output, the value actually set.
+
+  @retval RETURN_SUCCESS            The new attributes were set on
the serial device.
+  @retval RETURN_UNSUPPORTED        The serial device does not
support this operation.
+  @retval RETURN_INVALID_PARAMETER  One or more of the attributes
has an unsupported value.
+  @retval RETURN_DEVICE_ERROR       The serial device is not
functioning correctly.
+
+**/
+RETURN_STATUS
+EFIAPI
+SerialPortSetAttributes (
+  IN OUT UINT64             *BaudRate,
+  IN OUT UINT32             *ReceiveFifoDepth,
+  IN OUT UINT32             *Timeout,
+  IN OUT EFI_PARITY_TYPE    *Parity,
+  IN OUT UINT8              *DataBits,
+  IN OUT EFI_STOP_BITS_TYPE *StopBits
+  )
+{
+  return RETURN_UNSUPPORTED;
+}
+
diff --git
a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
index aced666..b73ab8f 100644
--- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
@@ -5,6 +5,7 @@
    Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR>
    Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR>
    Copyright (c) 2014, Red Hat, Inc.<BR>
+  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>

    This program and the accompanying materials
    are licensed and made available under the terms and conditions of
the BSD License
@@ -148,3 +149,89 @@ SerialPortPoll (
    }
    return FALSE;
  }
+
+/**
+  Sets the baud rate, receive FIFO depth, transmit/receice time out,
parity,
+  data bits, and stop bits on a serial device.
+
+  @param BaudRate           The requested baud rate. A BaudRate
value of 0 will use the
+                            device's default interface speed.
+                            On output, the value actually set.
+  @param ReveiveFifoDepth   The requested depth of the FIFO on the
receive side of the
+                            serial interface. A ReceiveFifoDepth
value of 0 will use
+                            the device's default FIFO depth.
+                            On output, the value actually set.
+  @param Timeout            The requested time out for a single
character in microseconds.
+                            This timeout applies to both the
transmit and receive side of the
+                            interface. A Timeout value of 0 will use
the device's default time
+                            out value.
+                            On output, the value actually set.
+  @param Parity             The type of parity to use on this serial
device. A Parity value of
+                            DefaultParity will use the device's
default parity value.
+                            On output, the value actually set.
+  @param DataBits           The number of data bits to use on the
serial device. A DataBits
+                            vaule of 0 will use the device's default
data bit setting.
+                            On output, the value actually set.
+  @param StopBits           The number of stop bits to use on this
serial device. A StopBits
+                            value of DefaultStopBits will use the
device's default number of
+                            stop bits.
+                            On output, the value actually set.
+
+  @retval RETURN_SUCCESS            The new attributes were set on
the serial device.
+  @retval RETURN_UNSUPPORTED        The serial device does not
support this operation.
+  @retval RETURN_INVALID_PARAMETER  One or more of the attributes
has an unsupported value.
+  @retval RETURN_DEVICE_ERROR       The serial device is not
functioning correctly.
+
+**/
+RETURN_STATUS
+EFIAPI
+SerialPortSetAttributes (
+  IN OUT UINT64             *BaudRate,
+  IN OUT UINT32             *ReceiveFifoDepth,
+  IN OUT UINT32             *Timeout,
+  IN OUT EFI_PARITY_TYPE    *Parity,
+  IN OUT UINT8              *DataBits,
+  IN OUT EFI_STOP_BITS_TYPE *StopBits
+  )
+{
+  return RETURN_UNSUPPORTED;
+}
+
+/**
+  Sets the control bits on a serial device.
+
+  @param Control                Sets the bits of Control that are
settable.
+
+  @retval RETURN_SUCCESS        The new control bits were set on the
serial device.
+  @retval RETURN_UNSUPPORTED    The serial device does not support
this operation.
+  @retval RETURN_DEVICE_ERROR   The serial device is not functioning
correctly.
+
+**/
+RETURN_STATUS
+EFIAPI
+SerialPortSetControl (
+  IN UINT32 Control
+  )
+{
+  return RETURN_UNSUPPORTED;
+}
+
+/**
+  Retrieve the status of the control bits on a serial device.
+
+  @param Control                A pointer to return the current
control signals from the serial device.
+
+  @retval RETURN_SUCCESS        The control bits were read from the
serial device.
+  @retval RETURN_UNSUPPORTED    The serial device does not support
this operation.
+  @retval RETURN_DEVICE_ERROR   The serial device is not functioning
correctly.
+
+**/
+RETURN_STATUS
+EFIAPI
+SerialPortGetControl (
+  OUT UINT32 *Control
+  )
+{
+  return RETURN_UNSUPPORTED;
+}
+


(2) although I see that you unified the GetControl / SetControl /
SetAttributes implementations between
- EarlyFdtPL011SerialPortLib and
- FdtPL011SerialPortLib,

the actual implementations are incorrect, plus this doesn't seem to be
what we agreed upon.

Please refer to:

http://thread.gmane.org/gmane.comp.bios.edk2.devel/4370/focus=4408

You wrote "Even the SerialDxe may not work for ConsoleIn (ConsoleOut
should work well) with the functions return RETURN_UNSUPPORTED
(especially SerialPortGetControl())."

So I think that that *all six* functions should return RETURN_SUCCESS.
(And both GetControl functions should also set *Control to zero.)

Again, please just do what
"EmbeddedPkg/Library/SerialPortExtLibNull/SerialPortExtLibNull.c" does.


There will be no difference to return RETURN_SUCCESS or
RETURN_UNSUPPORTED if the interfaces are not to do the real get/set
operation.

Oh, there is difference between RETURN_SUCCESS(with zeroed *Control) and 
RETURN_UNSUPPORTED. Zeroed *Control will let terminal driver think the 
input buffer is always not empty. It works for ConsoleIn, but only cause 
more useless operations to get the input data if in fact the input 
buffer is empty.

You still prefer this?

Thanks,
Star


The matter what I said is that the value return from
SerialPortGetControl() with *EFI_SERIAL_INPUT_BUFFER_EMPTY*, you can see
the line 568 in TerminalConIn.c. That is why I implemented the
interfaces for FdtPL011SerialPortLib in V2 patch series. And another
EarlyFdtPL011SerialPortLib does not need implement the interfaces or
just return RETURN_UNSUPPORTED as it does not link to SerialDxe, you can
also the implementation of SerialPortRead() and SerialPortPoll() in
EarlyFdtPL011SerialPortLib.c that do nothing.

 > You wrote "Even the SerialDxe may not work for ConsoleIn (ConsoleOut
 > should work well) with the functions return RETURN_UNSUPPORTED
 > (especially SerialPortGetControl())."

Thanks,
Star


(3) In connection, please adopt the last paragraph for the commit
message as well that I proposed last time:

     http://thread.gmane.org/gmane.comp.bios.edk2.devel/4370/focus=4406

     For now, we just copy the currently used code from
     "EmbeddedPkg/Library/SerialPortExtLibNull", for these three
     functions.

.... Actually, I'm now unsure if this was an oversight on your part, or
maybe the interface expectations changed (or got clarified).

Did they? Is it now okay for GetControl() to return RETURN_UNSUPPORTED?

In that case this patch is correct.

(I'm not worried about the other two functions, because your OvmfPkg
patch returns RETURN_UNSUPPORTED from those two functions as well.)

Thanks!
Laszlo


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