" On 15 January 2016 at 08:51, Zeng, Star <star.z...@intel.com> wrote: > On 2016/1/15 16:08, Ryan Harkin wrote: >> >> On 15 January 2016 at 06:50, Zeng, Star <star.z...@intel.com> wrote: >>> >>> On 2016/1/15 14:36, Ryan Harkin wrote: >>>> >>>> >>>> On 15 Jan 2016 01:41, "Zeng, Star" <star.z...@intel.com> wrote: >>>>> >>>>> >>>>> >>>>> Hi Ryan, >>>>> >>>>> >>>>> On 2016/1/15 3:10, Ryan Harkin wrote: >>>>>> >>>>>> >>>>>> >>>>>> Hi Star, >>>>>> >>>>>> This patch breaks the serial terminal for ARM FVP and Juno platforms. >>>>>> I assume it also breaks TC2 and other such "vexpress" platforms >>>>>> effected by this change. >>>>>> >>>>>> Whilst simple text input seems to work ok, cursor support does not. >>>>>> And we need cursor support for Intel BDS. >>>>>> >>>>>> Below is my hack at fixing the problem. >>>>>> >>>>>> Basically, I reintroduce PL011SerialPortExtLib and stub out the >>>>>> functionality in the functions copied into PL011SerialPortLib. >>>>> >>>>> >>>>> >>>>> >>>>> You just copied the function implementation back to >>>> >>>> >>>> PL011SerialPortExtLib, then it would work? It is so weird, >>>> >>>> Yes, that's exactly what I did! Very weird! >>>> >>>>> may I know where your fork? >>>>> >>>> >>>> I haven't pushed this branch yet because I had only just started moving >>>> to >>>> the latest EDK2 tree. But my branch is only tianocore plus >>>> OpenPlatformPkg and my patch below. I see the same problem on FVP >>>> models >>>> on upstream and Juno on upstream if I hack it to use IntelBDS. >>> >>> >>> >>> You means it works well with latest EDK2 TRUNK code if no any change? But >>> it >>> will not work any more after using IntelBDS? >>> >> >> No, it does not work well using EDK2 trunk code. Normal text entry >> works on trunk, it is cursor keys that are broken. Probably some >> other special keys are broken too. >> >> You can use FVP to test trunk. So one can test this problem using the >> free FVP Foundation model from ARM. >> >> But if you want to test Juno on trunk, you have to make a change to >> ArmJuno.dsc to use Intel BDS instead of ARM BDS. Just like this FVP >> change: >> >> f46ac5f 2015-09-01 ArmPlatformPkg/ArmVExpress-FVP: add support for >> the Intel BDS [Ard Biesheuvel] >> >> git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18378 >> 6f19259b-4bc3-4df7-8a09-765794883524 >> >> This is because Intel BDS using cursor keys, ARM BDS does not. > > > Sorry, I am not familiar with ARM platform and can only analyze the problem > by code review. Up to now, I have no idea about the reason since the code > implementation are same but just in different library instances. > > Could you push your code into your fork? Then we can get it to have a look. >
Sure, I've just pushed it: https://git.linaro.org/landing-teams/working/arm/edk2.git/shortlog/refs/heads/serialdxe-fix-005 It's in the "serialdxe-fix-005" branch. > >> >> >>> >>>> >>>>> Thanks, >>>>> Star >>>>> >>>>> >>>>>> >>>>>> Any suggestions about why this is broken or proposals of a proper fix >>>>>> are welcome. In the meantime, I guess I'll have to carry this patch >>>>>> in my own fork. >>>>>> >>>>>> Regards, >>>>>> Ryan. >>>>>> >>>>>> >>>>>> From 76352a589d5eaf6e9be28469aba63288f4defb25 Mon Sep 17 00:00:00 >>>>>> 2001 >>>>>> From: Ryan Harkin <ryan.har...@linaro.org> >>>>>> Date: Thu, 14 Jan 2016 18:41:54 +0000 >>>>>> Subject: [PATCH] Fix "ArmPlatformPkg: Use SerialDxe in MdeModulePkg >>>>>> instead of EmbeddedPkg" >>>>>> >>>>>> The SerialDxe patch below breaks ARM Ltd. platforms: >>>>>> >>>>>>> commit 921e987b2b26602dc85eaee856d494b97b6e02b0 >>>>>>> Author: Star Zeng <star.z...@intel.com> >>>>>>> Date: Thu Nov 26 08:51:05 2015 +0000 >>>>>>> >>>>>>> ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of >>>>>>> EmbeddedPkg >>>>>>> >>>>>>> It is also to integrate PL011SerialPortExtLib to >>>>>>> PL011SerialPortLib. >>>>>>> >>>>>>> Cc: Michael D Kinney <michael.d.kin...@intel.com> >>>>>>> Cc: Liming Gao <liming....@intel.com> >>>>>>> Cc: Leif Lindholm <leif.lindh...@linaro.org> >>>>>>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >>>>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>>>> Signed-off-by: Star Zeng <star.z...@intel.com> >>>>>>> Reviewed-by: Ard Biesheuvel <ard.biesheu...@linaro.org> >>>>>>> >>>>>>> git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18971 >>>> >>>> >>>> 6f19259b-4bc3-4df7-8a09-765794883524 >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> I found that also reverting the change in PL011SerialPortLib.c >>>>>> resulted >>>>>> in failure to compilate, so I simply changed the return values of the >>>>>> copied in functions to zero. >>>>>> >>>>>> Signed-off-by: Ryan Harkin <ryan.har...@linaro.org> >>>>>> --- >>>>>> ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc | 1 + >>>>>> .../PL011SerialPortLib/PL011SerialPortExtLib.c | 137 >>>> >>>> >>>> +++++++++++++++++++++ >>>>>> >>>>>> >>>>>> .../PL011SerialPortLib/PL011SerialPortExtLib.inf | 43 +++++++ >>>>>> .../PL011SerialPortLib/PL011SerialPortLib.c | 12 +- >>>>>> 4 files changed, 184 insertions(+), 9 deletions(-) >>>>>> create mode 100644 >>>>>> ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c >>>>>> create mode 100644 >>>>>> ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf >>>>>> >>>>>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc >>>>>> b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc >>>>>> index 1b8127d..19c1955 100644 >>>>>> --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc >>>>>> +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc >>>>>> @@ -95,6 +95,7 @@ >>>>>> # ARM PL011 UART Driver >>>>>> PL011UartLib|ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf >>>>>> >>>> >>>> >>>> SerialPortLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf >>>>>> >>>>>> >>>>>> + >>>> >>>> >>>> >>>> >>>> SerialPortExtLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf >>>>>> >>>>>> >>>>>> # ARM SP804 Dual Timer Driver >>>>>> TimerLib|ArmPlatformPkg/Library/SP804TimerLib/SP804TimerLib.inf >>>>>> >>>>>> diff --git >>>> >>>> >>>> a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c >>>>>> >>>>>> >>>>>> b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c >>>>>> new file mode 100644 >>>>>> index 0000000..c60e7a4 >>>>>> --- /dev/null >>>>>> +++ >>>>>> b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c >>>>>> @@ -0,0 +1,137 @@ >>>>>> +/** @file >>>>>> + Serial I/O Port library functions with no library >>>> >>>> >>>> constructor/destructor >>>>>> >>>>>> >>>>>> + >>>>>> + Copyright (c) 2012-2014, ARM Ltd. All rights reserved.<BR> >>>>>> + >>>>>> + This program and the accompanying materials >>>>>> + are licensed and made available under the terms and conditions of >>>>>> the BSD License >>>>>> + which accompanies this distribution. The full text of the license >>>>>> may be found at >>>>>> + http://opensource.org/licenses/bsd-license.php >>>>>> + >>>>>> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" >>>>>> BASIS, >>>>>> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS >>>>>> OR >>>> >>>> >>>> IMPLIED. >>>>>> >>>>>> >>>>>> + >>>>>> +**/ >>>>>> + >>>>>> +#include <Base.h> >>>>>> + >>>>>> +#include <Library/IoLib.h> >>>>>> +#include <Library/PcdLib.h> >>>>>> +#include <Library/SerialPortExtLib.h> >>>>>> + >>>>>> +#include <Drivers/PL011Uart.h> >>>>>> + >>>>>> +/** >>>>>> + Set new attributes to PL011. >>>>>> + >>>>>> + @param BaudRate The baud rate of the serial device. >>>>>> If the baud rate is not supported, >>>>>> + the speed will be reduced down to >>>>>> the nearest supported one and the >>>>>> + variable's value will be updated >>>> >>>> >>>> accordingly. >>>>>> >>>>>> >>>>>> + @param ReceiveFifoDepth The number of characters the device >>>>>> will buffer on input. If the specified >>>>>> + value is not supported, the >>>>>> variable's value will be reduced down to the >>>>>> + nearest supported one. >>>>>> + @param Timeout If applicable, the number of >>>>>> microseconds the device will wait >>>>>> + before timing out a Read or a Write >>>>>> operation. >>>>>> + @param Parity If applicable, this is the >>>>>> EFI_PARITY_TYPE that is computed or checked >>>>>> + as each character is transmitted or >>>>>> received. If the device does not >>>>>> + support parity, the value is the >>>>>> default parity value. >>>>>> + @param DataBits The number of data bits in each >>>> >>>> >>>> character >>>>>> >>>>>> >>>>>> + @param StopBits If applicable, the >>>>>> EFI_STOP_BITS_TYPE number of stop bits per character. >>>>>> + If the device does not support stop >>>>>> bits, the value is the default stop >>>>>> + bit value. >>>>>> + >>>>>> + @retval EFI_SUCCESS All attributes were set correctly >>>>>> on the serial device. >>>>>> + @retval EFI_INVALID_PARAMETERS One or more of the attributes has >>>>>> an unsupported value. >>>>>> + >>>>>> +**/ >>>>>> +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 PL011UartInitializePort ( >>>>>> + (UINTN)PcdGet64 (PcdSerialRegisterBase), >>>>>> + BaudRate, >>>>>> + ReceiveFifoDepth, >>>>>> + Parity, >>>>>> + DataBits, >>>>>> + StopBits); >>>>>> +} >>>>>> + >>>>>> +/** >>>>>> + >>>>>> + Assert or deassert the control signals on a serial port. >>>>>> + The following control signals are set according their bit settings >>>>>> : >>>>>> + . Request to Send >>>>>> + . Data Terminal Ready >>>>>> + >>>>>> + @param[in] Control The following bits are taken into account : >>>>>> + . EFI_SERIAL_REQUEST_TO_SEND : assert/deassert >>>> >>>> >>>> the >>>>>> >>>>>> >>>>>> + "Request To Send" control signal if this bit >>>>>> is >>>>>> + equal to one/zero. >>>>>> + . EFI_SERIAL_DATA_TERMINAL_READY : >>>> >>>> >>>> assert/deassert >>>>>> >>>>>> >>>>>> + the "Data Terminal Ready" control signal if >>>> >>>> >>>> this >>>>>> >>>>>> >>>>>> + bit is equal to one/zero. >>>>>> + . EFI_SERIAL_HARDWARE_LOOPBACK_ENABLE : >>>> >>>> >>>> enable/disable >>>>>> >>>>>> >>>>>> + the hardware loopback if this bit is equal >>>>>> to >>>>>> + one/zero. >>>>>> + . EFI_SERIAL_SOFTWARE_LOOPBACK_ENABLE : not >>>> >>>> >>>> supported. >>>>>> >>>>>> >>>>>> + . EFI_SERIAL_HARDWARE_FLOW_CONTROL_ENABLE : >>>> >>>> >>>> enable/ >>>>>> >>>>>> >>>>>> + disable the hardware flow control based on >>>>>> CTS >>>> >>>> >>>> (Clear >>>>>> >>>>>> >>>>>> + To Send) and RTS (Ready To Send) control >>>> >>>> >>>> signals. >>>>>> >>>>>> >>>>>> + >>>>>> + @retval RETURN_SUCCESS The new control bits were set on the >>>>>> serial device. >>>>>> + @retval RETURN_UNSUPPORTED The serial device does not support >>>>>> this operation. >>>>>> + >>>>>> +**/ >>>>>> +RETURN_STATUS >>>>>> +EFIAPI >>>>>> +SerialPortSetControl ( >>>>>> + IN UINT32 Control >>>>>> + ) >>>>>> +{ >>>>>> + return PL011UartSetControl ((UINTN)PcdGet64 >>>>>> (PcdSerialRegisterBase), Control); >>>>>> +} >>>>>> + >>>>>> +/** >>>>>> + >>>>>> + Retrieve the status of the control bits on a serial device. >>>>>> + >>>>>> + @param[out] Control Status of the control bits on a serial device >>>>>> : >>>>>> + >>>>>> + . EFI_SERIAL_DATA_CLEAR_TO_SEND, >>>>>> EFI_SERIAL_DATA_SET_READY, >>>>>> + EFI_SERIAL_RING_INDICATE, >>>> >>>> >>>> EFI_SERIAL_CARRIER_DETECT, >>>>>> >>>>>> >>>>>> + EFI_SERIAL_REQUEST_TO_SEND, >>>>>> EFI_SERIAL_DATA_TERMINAL_READY >>>>>> + are all related to the DTE (Data Terminal >>>>>> Equipment) and >>>>>> + DCE (Data Communication Equipment) modes of >>>>>> operation of >>>>>> + the serial device. >>>>>> + . EFI_SERIAL_INPUT_BUFFER_EMPTY : equal to >>>>>> one if the receive >>>>>> + buffer is empty, 0 otherwise. >>>>>> + . EFI_SERIAL_OUTPUT_BUFFER_EMPTY : equal to >>>>>> one if the transmit >>>>>> + buffer is empty, 0 otherwise. >>>>>> + . EFI_SERIAL_HARDWARE_LOOPBACK_ENABLE : equal >>>>>> to one if the >>>>>> + hardware loopback is enabled (the ouput >>>>>> feeds the receive >>>>>> + buffer), 0 otherwise. >>>>>> + . EFI_SERIAL_SOFTWARE_LOOPBACK_ENABLE : equal >>>>>> to one if a >>>>>> + loopback is accomplished by software, 0 >>>> >>>> >>>> otherwise. >>>>>> >>>>>> >>>>>> + . EFI_SERIAL_HARDWARE_FLOW_CONTROL_ENABLE : >>>>>> equal to one if the >>>>>> + hardware flow control based on CTS (Clear >>>>>> To Send) and RTS >>>>>> + (Ready To Send) control signals is enabled, >>>>>> 0 otherwise. >>>>>> + >>>>>> + @retval RETURN_SUCCESS The control bits were read from the serial >>>> >>>> >>>> device. >>>>>> >>>>>> >>>>>> + >>>>>> +**/ >>>>>> +RETURN_STATUS >>>>>> +EFIAPI >>>>>> +SerialPortGetControl ( >>>>>> + OUT UINT32 *Control >>>>>> + ) >>>>>> +{ >>>>>> + return PL011UartGetControl ((UINTN)PcdGet64 >>>>>> (PcdSerialRegisterBase), Control); >>>>>> +} >>>>>> diff --git >>>> >>>> >>>> a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf >>>>>> >>>>>> >>>>>> b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf >>>>>> new file mode 100644 >>>>>> index 0000000..38cdeee >>>>>> --- /dev/null >>>>>> +++ >>>>>> b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf >>>>>> @@ -0,0 +1,43 @@ >>>>>> +#/** @file >>>>>> +# >>>>>> +# Component description file for PL011SerialPortLib module >>>>>> +# >>>>>> +# Copyright (c) 2011-2012, ARM Ltd. All rights reserved.<BR> >>>>>> +# >>>>>> +# This program and the accompanying materials >>>>>> +# are licensed and made available under the terms and conditions of >>>>>> the BSD License >>>>>> +# which accompanies this distribution. The full text of the license >>>>>> may be found at >>>>>> +# http://opensource.org/licenses/bsd-license.php >>>>>> +# >>>>>> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" >>>>>> BASIS, >>>>>> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS >>>>>> OR IMPLIED. >>>>>> +# >>>>>> +#**/ >>>>>> + >>>>>> +[Defines] >>>>>> + INF_VERSION = 0x00010005 >>>>>> + BASE_NAME = PL011SerialPortExtLib >>>>>> + FILE_GUID = >>>>>> 2be281f1-c506-4558-bd98-d6930e6de9d6 >>>>>> + MODULE_TYPE = BASE >>>>>> + VERSION_STRING = 1.0 >>>>>> + LIBRARY_CLASS = SerialPortExtLib >>>>>> + >>>>>> +[Sources.common] >>>>>> + PL011SerialPortExtLib.c >>>>>> + >>>>>> +[LibraryClasses] >>>>>> + PL011UartLib >>>>>> + PcdLib >>>>>> + >>>>>> +[Packages] >>>>>> + EmbeddedPkg/EmbeddedPkg.dec >>>>>> + MdePkg/MdePkg.dec >>>>>> + MdeModulePkg/MdeModulePkg.dec >>>>>> + ArmPlatformPkg/ArmPlatformPkg.dec >>>>>> + >>>>>> +[Pcd] >>>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase >>>>>> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate >>>>>> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits >>>>>> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity >>>>>> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits >>>>>> diff --git >>>> >>>> >>>> a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c >>>>>> >>>>>> >>>>>> b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c >>>>>> index 7497b5e..8ab3dac 100644 >>>>>> --- a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c >>>>>> +++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c >>>>>> @@ -144,13 +144,7 @@ SerialPortSetAttributes ( >>>>>> IN OUT EFI_STOP_BITS_TYPE *StopBits >>>>>> ) >>>>>> { >>>>>> - return PL011UartInitializePort ( >>>>>> - (UINTN)PcdGet64 (PcdSerialRegisterBase), >>>>>> - BaudRate, >>>>>> - ReceiveFifoDepth, >>>>>> - Parity, >>>>>> - DataBits, >>>>>> - StopBits); >>>>>> + return 0; >>>>>> } >>>>>> >>>>>> /** >>>>>> @@ -185,7 +179,7 @@ SerialPortSetControl ( >>>>>> IN UINT32 Control >>>>>> ) >>>>>> { >>>>>> - return PL011UartSetControl ((UINTN)PcdGet64 >>>>>> (PcdSerialRegisterBase), Control); >>>>>> + return 0; >>>>>> } >>>>>> >>>>>> /** >>>>>> @@ -222,5 +216,5 @@ SerialPortGetControl ( >>>>>> OUT UINT32 *Control >>>>>> ) >>>>>> { >>>>>> - return PL011UartGetControl ((UINTN)PcdGet64 >>>>>> (PcdSerialRegisterBase), Control); >>>>>> + return 0; >>>>>> } >>>>>> >>>>> >>> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
