Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with PCD

2016-03-24 Thread Laszlo Ersek 
On 03/24/16 03:47, Ni, Ruiyu wrote:
> 
> 
> 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.

Okay, I understand. However, in that case, the series that reverts
31ae446b should first provide DSC patches that set this new PCD for
platforms that currently depend on 31ae446b, and revert 31ae446b second.

In other words, the testing that Ryan did thus far wasn't correct --
simply because we didn't ask him to test the right thing. The right
thing would be to (a) apply Heyi's patches, (b) set the new PCD in his
platform DSC(s), (c) revert 31ae446b, (d) test the cursor keys.

Unfortunately, I don't know what exact value Ryan should set in this new
PCD. I can imagine it cannot even be set in a platform DSC flexibly
enough, i.e., maybe it should be set dynamically in some PEIM.

Thanks
Laszlo

> 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."
>>>
>>>

Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with PCD

2016-03-24 Thread Ni, Ruiyu 
>>
>>> -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

Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with PCD

2016-03-24 Thread Heyi Guo 



On 03/24/2016 10:47 AM, Ni, Ruiyu wrote:


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.


Sorry I misunderstood your previous email. Does it make sense to set the 
default value of PCD to 0 directly?




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?

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/MdeMo

Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with PCD

2016-03-23 Thread Ni, Ruiyu 


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
>>>>>

Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with PCD

2016-03-23 Thread Ni, Ruiyu 


Regards,
Ray


>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
>Ersek
>Sent: Wednesday, March 23, 2016 4:53 PM
>To: Ni, Ruiyu <ruiyu...@intel.com>
>Cc: Tian, Feng <feng.t...@intel.com>; Ard Biesheuvel 
><ard.biesheu...@linaro.org>; edk2-devel@lists.01.org
><edk2-de...@ml01.01.org>; Heyi Guo <heyi@linaro.org>; Ryan Harkin 
><ryan.har...@linaro.org>; Zeng, Star
><star.z...@intel.com>
>Subject: Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with 
>PCD
>
>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 the FIFO depth is 16, we can just set the newly added
PcdUartDefaultReceiveFIfoDepth to 16 to test the functionality.
Rather than waiting the 3/3 terminal polling rate adjustment patch.

>
>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.
>
>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  = (UINT3

Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with PCD

2016-03-23 Thread Heyi Guo 

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.


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->D

Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with PCD

2016-03-23 Thread Laszlo Ersek 
On 03/23/16 17:26, 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'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."

Thank you very much for the feedback.

Ray, can you please work with Heyi, Ryan and others to analyze why this
series of Heyi's doesn't supplant 31ae446b?

And, until that is determined, could you please hold off on reverting
31ae446b?

Thank you,
Laszlo




>>
>> 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.Pcd

Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with PCD

2016-03-23 Thread Ryan Harkin 
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'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->

Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with PCD

2016-03-23 Thread Ryan Harkin 
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.

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);
>>>

Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with PCD

2016-03-23 Thread Ni, Ruiyu 
Thanks for the understanding.

I will post the patch to revert the change.

Regards,
Ray


>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
>Ersek
>Sent: Wednesday, March 23, 2016 4:53 PM
>To: Ni, Ruiyu <ruiyu...@intel.com>
>Cc: Tian, Feng <feng.t...@intel.com>; Ard Biesheuvel 
><ard.biesheu...@linaro.org>; edk2-devel@lists.01.org
><edk2-de...@ml01.01.org>; Heyi Guo <heyi@linaro.org>; Ryan Harkin 
><ryan.har...@linaro.org>; Zeng, Star
><star.z...@intel.com>
>Subject: Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with 
>PCD
>
>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.
>
>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

Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with PCD

2016-03-23 Thread Laszlo Ersek 
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.

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

Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with PCD

2016-03-23 Thread Ni, Ruiyu 
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?

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] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with PCD

2016-03-19 Thread Heyi Guo 
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 
Cc: Feng Tian 
Cc: Star Zeng 
---
 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