>> >>> -----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. > >Sorry I misunderstood your previous email. Does it make sense to set the >default value of PCD to 0 directly?
This PCD is to let platform tell SerialDxe driver the Receive FIFO depth. So I think setting 0 doesn't make sense. > >> >> And I will still talk with Heyi to understand why 3/3 timer interval patch >> needs to >> coded like that. > >Did you see my early mail to explain the calculation? Yes I saw it. Let's talk in that mail thread. > >Regards. > >Heyi > >>> 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
