On 11/17/15 11:55, Zeng, Star wrote:
> On 2015/11/17 18:09, Laszlo Ersek wrote:
>> On 11/17/15 10:35, Star Zeng wrote:
>>> It is also to add GetControl/SetControl/SetAttributes implementation
>>> for EarlyFdtPL011SerialPortLib and FdtPL011SerialPortLib.
>>>
>>> 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 | 82
>>> +++++++++++++++++-
>>> .../FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 99
>>> ++++++++++++++++++++++
>>> 7 files changed, 186 insertions(+), 6 deletions(-)
>>
>> I think that the FdtPL011SerialPortLib changes (i.e., the C file
>> changes) should be in a separate patch.
>>
>> For the DSC and FDF changes only:
>>
>> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
>>
>> I'll leave the review of the separate patch (with the C file changes) to
>> Ard.
>>
>> Thanks
>> Laszlo
>
> I am ok to separate the c file changes to another patch.
> But I guess that may break the bisect of ArmVirtPkg , although I didnot
> try that.
>
> Do you really want that ?
Hm, no. You are correct.
I didn't miss the bisection aspect, but I thought (incorrectly) that the
library changes were separable from the driver flipping.
If I understand right, that is not the case.
Therefore, can you please:
(1) Reword the commit message as follows:
--------
ArmVirtPkg: Use SerialDxe from MdeModulePkg instead of EmbeddedPkg
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.
For now, we just copy the currently used code from
"EmbeddedPkg/Library/SerialPortExtLibNull", for these three functions.
--------
(2) Regarding the actual implementation of these three functions, in
EarlyFdtPL011SerialPortLib and FdtPL011SerialPortLib -- see the last
paragraph above --, I *think* that in this patch you should just copy
the original behavior from
"EmbeddedPkg/Library/SerialPortExtLibNull/SerialPortExtLibNull.c".
Then each library instance can be separately improved (with individual
patches), *if* we want them to return RETURN_UNSUPPORTED (rather than
RETURN_SUCCESS), or to expose PL011Uart*() functions from PL011UartLib.
But, for now, this patch should not change the behavior of the protocol
member functions that the (new) driver exposes.
Thanks!
Laszlo
>
> Thanks,
> Star
>
>>
>>>
>>> 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..c02579a 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,83 @@ 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 buts, 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.
>>> + @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.
>>> + @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.
>>> + @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.
>>> + @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.
>>> + @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.
>>> +
>>> + @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..2d065fe 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,101 @@ SerialPortPoll (
>>> }
>>> return FALSE;
>>> }
>>> +
>>> +/**
>>> + Sets the baud rate, receive FIFO depth, transmit/receice time out,
>>> parity,
>>> + data buts, 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.
>>> + @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.
>>> + @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.
>>> + @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.
>>> + @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.
>>> + @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.
>>> +
>>> + @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
>>> + )
>>> +{
>>> + if (mSerialBaseAddress != 0) {
>>> + return PL011UartInitializePort (
>>> + mSerialBaseAddress,
>>> + BaudRate,
>>> + ReceiveFifoDepth,
>>> + Parity,
>>> + DataBits,
>>> + StopBits);
>>> + } else {
>>> + 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
>>> + )
>>> +{
>>> + if (mSerialBaseAddress != 0) {
>>> + return PL011UartSetControl (mSerialBaseAddress, Control);
>>> + } else {
>>> + 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
>>> + )
>>> +{
>>> + if (mSerialBaseAddress != 0) {
>>> + return PL011UartGetControl (mSerialBaseAddress, Control);
>>> + } else {
>>> + return RETURN_UNSUPPORTED;
>>> + }
>>> +}
>>> +
>>>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
