Regards, Ray
>-----Original Message----- >From: Heyi Guo [mailto:heyi....@linaro.org] >Sent: Thursday, March 24, 2016 9:09 AM >To: Ryan Harkin <ryan.har...@linaro.org>; Laszlo Ersek <ler...@redhat.com> >Cc: Ni, Ruiyu <ruiyu...@intel.com>; Tian, Feng <feng.t...@intel.com>; >edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; >Zeng, Star <star.z...@intel.com>; Ard Biesheuvel <ard.biesheu...@linaro.org> >Subject: Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with >PCD > >Hi folks, > >Please see my thoughts below: > >On 03/24/2016 12:26 AM, Ryan Harkin wrote: >> On 23 March 2016 at 16:24, Ryan Harkin <ryan.har...@linaro.org> wrote: >>> On 23 March 2016 at 08:53, Laszlo Ersek <ler...@redhat.com> wrote: >>>> On 03/23/16 09:33, Ni, Ruiyu wrote: >>>>> Laszlo, >>>>> Since the patch below lets SerialDxe driver use the new introduced >>>>> PCD gEfiMdePkgTokenSpaceGuid.PcdUartDefaultReceiveFifoDepth. >>>>> Would you mind to revert the check in @ 31ae446b >>>>> --> MdeModulePkg: TerminalDxe: select the UART's default receive FIFO >>>>> depth? >>>> If I understand correctly, this series increases the terminal polling >>>> rate, so that cursor movement escape sequences (and other burst-like >>>> sequences) can be processed even with a receive fifo depth of 1. Hence >>>> 31ae446b should become unnecessary. >>>> >>>> If Ryan and Ard (CC'd), who tested 31ae446b originally, can also test >>>> your revert of 31ae446b -- on top of this series from Heyi --, and the >>>> cursor keys work with 31ae446b reverted, then I don't mind, sure. >>>> >>> Unfortunately TC2 and Juno no longer with with 31ae446b reverted and >>> these 3 patches from the series applied. > >I think my patches may not be sufficient to replace patch 31ae446b; they >have some different results. > >The result of patch 31ae446b is absolute; it does not rely on timer >event or code execution but only on UART hardware. > >My patches are actually complementary software solution for platforms >which have certain length of FIFO depth, e.g. 40 characters continuous >input to UART with 32 FIFO depth. I don't think it will work well on >FIFO depth of 1, as it does not have enough scalability for software >processing. This also the reason why I reduce the precisely calculated >interval. > >It also depends on the timer interrupt period (should be >gEmbeddedTokenSpaceGuid.PcdTimerPeriod on ARM platforms), which must be >less than the calculated polling interval. > >So my opinion is to keep patch 31ae446b if it is reasonable on its own, >and take my patches as further enhancement. NO. I understand the fact that the patch 31ae446b did fix an issue in certain platform. But the patch lets TerminalDxe driver changes the underlying UART's Receive FIFO depth. Such behavior is bad. Now since we have added the new PCD PcdDefaultUartReceiveFifoDepth, we can fix the original problem by setting the PCD value to 16 from platform DSC. In all, I wanted to say, with the new PCD patch, the patch 31ae446b can be reverted without functionality impact. And I will still talk with Heyi to understand why 3/3 timer interval patch needs to coded like that. > >Thanks. > >Heyi > > >> I'm sure you would all work it out ok, but that should have been: >> >> "Unfortunately TC2 and Juno no longer work with 31ae446b reverted and >> these 3 patches from the series applied." >> >> >>> It's a shame, because it works on FVP models and copy/paste then works. >>> >>> I haven't made any investigations on what's going wrong. But regular >>> ASCII keys work, control codes like cursor keys don't. So it looks >>> like the old FIFO setting problem we discussed a few weeks ago. >>> >>> >>>> Thanks >>>> Laszlo >>>> >>>>> Regards, >>>>> Ray >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >>>>>> Heyi Guo >>>>>> Sent: Thursday, March 17, 2016 10:37 PM >>>>>> To: edk2-devel@lists.01.org >>>>>> Cc: Heyi Guo <heyi....@linaro.org>; Tian, Feng <feng.t...@intel.com>; >>>>>> Zeng, Star <star.z...@intel.com> >>>>>> Subject: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth >>>>>> with PCD >>>>>> >>>>>> Set UART receive FIFO depth with PCD instead of fixed number "1". >>>>>> The default value of PCD is also 1, so it makes no difference for >>>>>> platforms which do not explicitly set this PCD. >>>>>> >>>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>>> Signed-off-by: Heyi Guo <heyi....@linaro.org> >>>>>> Cc: Feng Tian <feng.t...@intel.com> >>>>>> Cc: Star Zeng <star.z...@intel.com> >>>>>> --- >>>>>> MdeModulePkg/Universal/SerialDxe/SerialDxe.inf | 9 +++++---- >>>>>> MdeModulePkg/Universal/SerialDxe/SerialIo.c | 3 ++- >>>>>> 2 files changed, 7 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf >>>>>> b/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf >>>>>> index 164060b..a1453bd 100644 >>>>>> --- a/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf >>>>>> +++ b/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf >>>>>> @@ -41,10 +41,11 @@ >>>>>> gEfiDevicePathProtocolGuid ## PRODUCES >>>>>> >>>>>> [Pcd] >>>>>> - gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate ## CONSUMES >>>>>> - gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits ## CONSUMES >>>>>> - gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity ## CONSUMES >>>>>> - gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits ## CONSUMES >>>>>> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate ## CONSUMES >>>>>> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits ## CONSUMES >>>>>> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity ## CONSUMES >>>>>> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits ## CONSUMES >>>>>> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultReceiveFifoDepth ## CONSUMES >>>>>> >>>>>> [Depex] >>>>>> TRUE >>>>>> diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c >>>>>> b/MdeModulePkg/Universal/SerialDxe/SerialIo.c >>>>>> index f5b3064..d2383e5 100644 >>>>>> --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c >>>>>> +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c >>>>>> @@ -236,7 +236,7 @@ SerialReset ( >>>>>> // >>>>>> // Set the Serial I/O mode >>>>>> // >>>>>> - This->Mode->ReceiveFifoDepth = 1; >>>>>> + This->Mode->ReceiveFifoDepth = PcdGet16 >>>>>> (PcdUartDefaultReceiveFifoDepth); >>>>>> This->Mode->Timeout = 1000 * 1000; >>>>>> This->Mode->BaudRate = PcdGet64 (PcdUartDefaultBaudRate); >>>>>> This->Mode->DataBits = (UINT32) PcdGet8 >>>>>> (PcdUartDefaultDataBits); >>>>>> @@ -508,6 +508,7 @@ SerialDxeInitialize ( >>>>>> mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits); >>>>>> mSerialIoMode.Parity = (UINT32) PcdGet8 (PcdUartDefaultParity); >>>>>> mSerialIoMode.StopBits = (UINT32) PcdGet8 (PcdUartDefaultStopBits); >>>>>> + mSerialIoMode.ReceiveFifoDepth = PcdGet16 >>>>>> (PcdUartDefaultReceiveFifoDepth); >>>>>> mSerialDevicePath.Uart.BaudRate = PcdGet64 (PcdUartDefaultBaudRate); >>>>>> mSerialDevicePath.Uart.DataBits = PcdGet8 (PcdUartDefaultDataBits); >>>>>> mSerialDevicePath.Uart.Parity = PcdGet8 (PcdUartDefaultParity); >>>>>> -- >>>>>> 2.7.0 >>>>>> >>>>>> _______________________________________________ >>>>>> 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 >>>>> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
